API Changes: TargetMachine::getSubtarget

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

API Changes: TargetMachine::getSubtarget

Eric Christopher
Hi all,

As of r232885 I've removed the argument-less TargetMachine::getSubtarget and TargetMachine::getSubtargetImpl. For the targets that aren't completely independent of this I've gone ahead and left a non-virtual version of the function in the target specific TargetMachine. What this means in practice is that those targets can only use a bare getSubtarget call in their target specific code, and should probably clean that up as soon as they can. They'll definitely be unable to use any of the function multiversioning support when it goes in and they'll be unable to LTO binaries that have functions with different subtarget features/cpus.

Updating out of tree ports should be fairly straightforward for now. Updating out of tree code otherwise should be ok, but given I've been removing this stuff for some time you've probably been working on it anyways. Do file bugs if there's anything I've missed or send me a message if you run into a problem (I know of a couple off hand that need investigating).

Thanks!

-eric

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: API Changes: TargetMachine::getSubtarget

David Blaikie


On Fri, Mar 20, 2015 at 9:35 PM, Eric Christopher <[hidden email]> wrote:
Hi all,

As of r232885 I've removed the argument-less TargetMachine::getSubtarget and TargetMachine::getSubtargetImpl.

Congrats!
 
For the targets that aren't completely independent of this I've gone ahead and left a non-virtual version of the function in the target specific TargetMachine. What this means in practice is that those targets can only use a bare getSubtarget call in their target specific code, and should probably clean that up as soon as they can. They'll definitely be unable to use any of the function multiversioning support when it goes in and they'll be unable to LTO binaries that have functions with different subtarget features/cpus.

Updating out of tree ports should be fairly straightforward for now. Updating out of tree code otherwise should be ok, but given I've been removing this stuff for some time you've probably been working on it anyways. Do file bugs if there's anything I've missed or send me a message if you run into a problem (I know of a couple off hand that need investigating).

Thanks!

-eric

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: API Changes: TargetMachine::getSubtarget

Akira Hatanaka
In reply to this post by Eric Christopher
On Fri, Mar 20, 2015 at 9:35 PM, Eric Christopher <[hidden email]> wrote:
Hi all,

As of r232885 I've removed the argument-less TargetMachine::getSubtarget and TargetMachine::getSubtargetImpl. For the targets that aren't completely independent of this I've gone ahead and left a non-virtual version of the function in the target specific TargetMachine. What this means in practice is that those targets can only use a bare getSubtarget call in their target specific code, and should probably clean that up as soon as they can. They'll definitely be unable to use any of the function multiversioning support when it goes in and they'll be unable to LTO binaries that have functions with different subtarget features/cpus.


Thanks!
 
Updating out of tree ports should be fairly straightforward for now. Updating out of tree code otherwise should be ok, but given I've been removing this stuff for some time you've probably been working on it anyways. Do file bugs if there's anything I've missed or send me a message if you run into a problem (I know of a couple off hand that need investigating).


I noticed LLVMTargetMachine::addPassesToEmitFile is passing the default MCSubtargetInfo to createMCInstPrinter. Since there are cases where AArch64's InstPrinter looks at the target cpu to print an operand (see AArch64SysReg::SysRegMapper::toString, which is called from AArch64InstPrinter::printMRSSystemRegister), should we make changes in AArch64AsmPrinter to pass down the per-function subtarget? You can compile the attached IR with llc and see it doesn't output cyclone's system register unless "-mcpu cyclone" is specified on the command line, even thought the IR has "target-cpu"="cyclone" on the function. I don't think this affects correctness, but I thought I should point out the difference.


Thanks!

-eric

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

instrprinter1.ll (804 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: API Changes: TargetMachine::getSubtarget

Eric Christopher


On Mon, Mar 23, 2015 at 3:54 PM Akira Hatanaka <[hidden email]> wrote:
On Fri, Mar 20, 2015 at 9:35 PM, Eric Christopher <[hidden email]> wrote:
Hi all,

As of r232885 I've removed the argument-less TargetMachine::getSubtarget and TargetMachine::getSubtargetImpl. For the targets that aren't completely independent of this I've gone ahead and left a non-virtual version of the function in the target specific TargetMachine. What this means in practice is that those targets can only use a bare getSubtarget call in their target specific code, and should probably clean that up as soon as they can. They'll definitely be unable to use any of the function multiversioning support when it goes in and they'll be unable to LTO binaries that have functions with different subtarget features/cpus.


Thanks!
 
Updating out of tree ports should be fairly straightforward for now. Updating out of tree code otherwise should be ok, but given I've been removing this stuff for some time you've probably been working on it anyways. Do file bugs if there's anything I've missed or send me a message if you run into a problem (I know of a couple off hand that need investigating).


I noticed LLVMTargetMachine::addPassesToEmitFile is passing the default MCSubtargetInfo to createMCInstPrinter. Since there are cases where AArch64's InstPrinter looks at the target cpu to print an operand (see AArch64SysReg::SysRegMapper::toString, which is called from AArch64InstPrinter::printMRSSystemRegister), should we make changes in AArch64AsmPrinter to pass down the per-function subtarget? You can compile the attached IR with llc and see it doesn't output cyclone's system register unless "-mcpu cyclone" is specified on the command line, even thought the IR has "target-cpu"="cyclone" on the function. I don't think this affects correctness, but I thought I should point out the difference.


Yep. ARM is in a similar state and well, as I said in the followup to the commit that added it:

"I hope it's temporary, but rewriting more huge swaths of the target infrastructure in both the TargetStreamer (mips) and/or the AsmPrinter which does this exact thing for a very similar reason is less on correctness and more on API level. Fixing this inside both of those is going to take some major reworking that may involve fairly significant internal API changes. So, we workaround for the moment so we can ensure the middle end code generation is clean and go from there."

I really don't want to talk about ARM's use. It's mostly that keeping the middle end clean was getting to be annoying and this temporary hack is a decent workaround.

Basically it's going to be a lot of churn to fix it and I was already in the middle of breaking lots of out of tree things so I figured I'd save it for another week :)

-eric 
 

Thanks!

-eric

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev


_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: API Changes: TargetMachine::getSubtarget

Akira Hatanaka
On Mon, Mar 23, 2015 at 4:04 PM, Eric Christopher <[hidden email]> wrote:


On Mon, Mar 23, 2015 at 3:54 PM Akira Hatanaka <[hidden email]> wrote:
On Fri, Mar 20, 2015 at 9:35 PM, Eric Christopher <[hidden email]> wrote:
Hi all,

As of r232885 I've removed the argument-less TargetMachine::getSubtarget and TargetMachine::getSubtargetImpl. For the targets that aren't completely independent of this I've gone ahead and left a non-virtual version of the function in the target specific TargetMachine. What this means in practice is that those targets can only use a bare getSubtarget call in their target specific code, and should probably clean that up as soon as they can. They'll definitely be unable to use any of the function multiversioning support when it goes in and they'll be unable to LTO binaries that have functions with different subtarget features/cpus.


Thanks!
 
Updating out of tree ports should be fairly straightforward for now. Updating out of tree code otherwise should be ok, but given I've been removing this stuff for some time you've probably been working on it anyways. Do file bugs if there's anything I've missed or send me a message if you run into a problem (I know of a couple off hand that need investigating).


I noticed LLVMTargetMachine::addPassesToEmitFile is passing the default MCSubtargetInfo to createMCInstPrinter. Since there are cases where AArch64's InstPrinter looks at the target cpu to print an operand (see AArch64SysReg::SysRegMapper::toString, which is called from AArch64InstPrinter::printMRSSystemRegister), should we make changes in AArch64AsmPrinter to pass down the per-function subtarget? You can compile the attached IR with llc and see it doesn't output cyclone's system register unless "-mcpu cyclone" is specified on the command line, even thought the IR has "target-cpu"="cyclone" on the function. I don't think this affects correctness, but I thought I should point out the difference.


Yep. ARM is in a similar state and well, as I said in the followup to the commit that added it:

"I hope it's temporary, but rewriting more huge swaths of the target infrastructure in both the TargetStreamer (mips) and/or the AsmPrinter which does this exact thing for a very similar reason is less on correctness and more on API level. Fixing this inside both of those is going to take some major reworking that may involve fairly significant internal API changes. So, we workaround for the moment so we can ensure the middle end code generation is clean and go from there."

I really don't want to talk about ARM's use. It's mostly that keeping the middle end clean was getting to be annoying and this temporary hack is a decent workaround.

Basically it's going to be a lot of churn to fix it and I was already in the middle of breaking lots of out of tree things so I figured I'd save it for another week :)


Ok, that makes sense. It looks like this hack would work for most normal use cases, so it should be fine for now.

-eric 
 

Thanks!

-eric

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev