<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 26, 2015 at 9:53 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Akira,<div><br></div><div>A bit of a follow up to my earlier email with some review on the patches themselves. Some of them I think are good and some I think need some thought.</div><div><br></div><div><div>+ Options.CPUStr = MCPU;</div><div>+ Options.FeaturesStr = MAttrs;</div><div><br></div><div>I'd definitely like to avoid having the CPU and Feature string on TargetOptions. Ideally the only things that are going to be on there are global options that can't be anywhere else - and I'd really like to investigate module flags here as well. I know we've had some discussion on that, and I've got my own opinions, but in general anything that's per function needs to be kept on the functions and anything that's general should be kept on the module.</div><div><br></div></div></div></blockquote><div><br></div><div>I added CPU and Feature string to TargetOptions to enable tools like llc to override the function attributes in the IR with command line options. The code which does subtarget lookup in X86TargetMachine::getSubtargetImpl(const Function&) in won't pick the CPU string in TargetOptions if there is a function attribute "target-cpu" in the IR. This means if you try to compile an IR that has function attribute "target-cpu" == "cpu1" with command "llc -mcpu=cpu2 ...", "cpu1" will be used to construct subtarget, and that's not what users of llc expect (at least that's my understanding). The TargetOptions::Option class has a bit to indicate whether the command line options was specified, which enables the function attribute to be overridden (see function llvm::getFnAttributeVal in the patch).</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div></div><div><div>+ template <typename STC> const STC &getSubtarget(const Function *F) const {</div><div>+ return *static_cast<const STC*>(getSubtargetImpl(*F));</div></div><div><br></div><div>Right. Though you missed getSubtargetImpl above, etc. A lot of the passes and backends use it rather than the other way around. I'm pulling them out and more help is definitely good. As I said in my first email perhaps the TTI->FTTI transition is a good place to start working here without having to worry much about redoing huge swaths of llvm.</div><div><br></div></div></div></blockquote><div><br></div><div>Yes, getSubtargetImpl should be fixed too.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div></div><div><div>- : Fn(F), Target(TM), STI(TM.getSubtargetImpl()), Ctx(mmi.getContext()),</div><div>+ : Fn(F), Target(TM), STI(TM.getSubtargetImpl(*F)), Ctx(mmi.getContext()),</div></div><div><br></div><div>This is obviously fine, but I've been avoiding this so that we don't have partial implementations in flight. I.e. if you do this then code that looks like what we want in the future will start being ... halfway code generated.</div><div><br></div><div><div>+ // Find the first function defined in the module and copy the cpu and</div><div>+ // feature string to Options.</div></div><div><br></div><div>Very much against this.</div><div><br></div></div></div></blockquote><div><br></div><div>I'm trying to avoid passing an empty CPU and features string to createTargetMachine (and constructor of the module-level subtarget) here. This won't be needed if the module-level subtarget aspect is going to be removed completely.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div></div><div> // FIXME: This should stop caching the target machine as soon as</div><div> // we can remove resetOperationActions et al.</div><div>-X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM)</div><div>- : TargetLowering(TM) {</div><div>- Subtarget = &TM.getSubtarget<X86Subtarget>();</div><div>+X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,</div><div>+ const X86Subtarget *ST)</div><div>+ : TargetLowering(TM), Subtarget(ST) {</div><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px">If we get rid of resetOperationActions then we don't need to do this and it only takes the X86Subtarget here.</span></div><div><span style="font-size:13.1999998092651px"><br></span></div></div></div></blockquote><div><br></div><div>OK. I see there is more work to do here.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><span style="font-size:13.1999998092651px"></span></div><div>- : LLVMTargetMachine(T, TT, CPU, FS, Options, RM, CM, OL),</div><div>+ : LLVMTargetMachine(T, TT, Options.CPUStr.val(), Options.FeaturesStr.val(),</div><div>+ Options, RM, CM, OL),</div><div> TLOF(createTLOF(Triple(getTargetTriple()))),</div><div> DL(computeDataLayout(Triple(TT))),</div><div>- Subtarget(TT, CPU, FS, *this, Options.StackAlignmentOverride) {</div><div>+ Subtarget(TT, Options.CPUStr, Options.FeaturesStr, *this,</div><div>+ Options.StackAlignmentOverride) {</div><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px">Again, I don't think this is the right way to go. The default CPU and FS can quite easily just be what's default in the module, there's no need to do this.</span></div><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px">Anyhow, it's definitely the direction I've been headed, I think there's some disconnect on how various things related to options should go and there's a lot of work that can be done in the meantime. I'll see what I can do to write up a more detailed plan of attack and start listing good places to start working on this (or complicated and hard ones if you'd like, there are a few of those left as well!)</span></div><div><br></div></div></div></blockquote><div><br></div><div>Yes, a detailed plan would definitely help, although I think it's getting a little clearer what needs to be done.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><span style="font-size:13.1999998092651px"></span></div><div><span style="font-size:13.1999998092651px">Thanks!</span></div><span><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px">-eric</span></div><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px"><br></span></div><div><span style="font-size:13.1999998092651px">On Mon Jan 26 2015 at 4:28:41 PM Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:</span><br></div></span><div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>I've been investigating what is needed to ensure command line options are passed to the backend codegen passes during LTO and enable compiling different functions in a module with different command line options (see the links below for previous discussions).<br></div><div><br></div><div><a href="http://thread.gmane.org/gmane.comp.compilers.llvm.devel/78855" target="_blank">http://thread.gmane.org/gmane.comp.compilers.llvm.devel/78855</a><br></div><div><a href="http://thread.gmane.org/gmane.comp.compilers.llvm.devel/80456" target="_blank">http://thread.gmane.org/gmane.comp.compilers.llvm.devel/80456</a><br></div><div><br></div><div>The command line options I'm currently looking into are "-target-cpu" and "-target-feature" and I would like to get feedback about the approach I've taken (patches attached).</div><div><br></div></div></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div>The attached patches make the following changes:</div><div><br></div><div>- In TargetMachine::getSubtarget(const Function*) and MachineFunction's constructor, use per-function subtarget object instead of TargetMachine's (module-level) subtarget object. This allows passes like selection dag to switch the target on a per-function basis.<br></div><div><br></div><div>- Define class TargetOptions::Option, which records whether an option has appeared on the command line along with the option's value. Long term, this might not be the best solution and I expect it will be modified or replaced when the new command line infrastructure becomes available.</div><div><br></div><div>- Fix X86's subtarget lookup to override the function attributes if the corresponding options were specified on the command line.</div><div><br></div><div>- FIx clang to embed "-target-cpu" and "-target-feature" attributes in the IR.</div><div><br></div><div>I've tested the changes I made and confirmed that target options such as "-mavx2" don't get dropped during LTO and are honored by backend codegen passes.</div><div><br></div><div>This is my plan for the remaining tasks:</div><div><br></div><div>1. FIx other in-tree targets and other code-gen passes that are still using TargetMachine's subtarget where the per-function subtarget should be used.</div><div><br></div><div>2. Fix TargetTransformInfo to compute the various code-gen costs accurately when subtarget is switched on a per-function basis. One way to do this is to make the pointer or reference to the Function object available to the various subclasses of TargetTransformInfo by defining the necessary functions in FunctionTargetTransformInfo (similar to the changes made in r218004). However, passes like Inliner that are not function passes cannot access FunctionTargetTransformInfo, so it has to be done in a different way.</div><div><br></div><div>3. Forbid inlining functions that have incompatible cpu and feature attributes. It seems the simplest approach is to allow inlining only if the cpu and feature attributes match exactly, but it's also possible to relax this restriction.</div></div>
</blockquote></div></div></div></div></div>
</blockquote></div><br></div></div>