<div dir="ltr">Perhaps the comments I left weren't very clear, but the plan was to add a subtarget feature to all targets for now and remove it later when we add support for "generic" subtarget features, which I believe will be something that will belong to TargetSubtargetInfo (Eric has probably spent more time thinking about this).<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 13, 2015 at 5:52 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">----- Original Message -----<br>
> From: "Eric Christopher" <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
> To: <a href="mailto:reviews%2BD11815%2Bpublic%2B324fadcdaae02561@reviews.llvm.org">reviews+D11815+public+324fadcdaae02561@reviews.llvm.org</a>, <a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>, <a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>,<br>
> <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a><br>
> Cc: <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
> Sent: Thursday, August 13, 2015 7:39:53 PM<br>
> Subject: Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"<br>
><br>
> On Thu, Aug 13, 2015 at 5:16 PM <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> hfinkel added a comment.<br>
><br>
</span><span class="">> In <a href="http://reviews.llvm.org/D11815#224169" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11815#224169</a> , @echristo wrote:<br>
><br>
> > No, RESET_OPTION isn't the right way to do this either. For exactly<br>
> > this sort of reason. You can't actually represent all of the code<br>
> > and options this way in the IR. If you can't do that then it's a<br>
> > non-starter.<br>
><br>
><br>
> Can you please be more specific, what is it that we cannot represent?<br>
> TargetOptions represents the target-generic code-generation options<br>
> for the current function (where the "current function" bit works<br>
> because the options get reset based on the current function<br>
> attributes). That's the design we have now. And sticking with that<br>
> pattern, even if we're going to change the overall scheme later, is<br>
> better than the code duplication and inconsistency proposed here.<br>
><br>
> Apologies, I'm really resistant to more things being used in<br>
> TargetOptions and I was (perhaps mistakenly) under the impression<br>
> that you wanted to move it to TargetOptions without an IR<br>
> serialization. We need all options to have that sort of<br>
> serialization right? :)<br>
<br>
</span>Absolutely, they all need function-level serialization for LTO to work. We're definitely both on the same page there :)<br>
<span class=""><br>
> In this case it's for the -mstackrealign<br>
> option and we need to keep that if it's going to work for separate<br>
> compilation.<br>
><br>
><br>
> I'm guessing from the comment here that you're talking about<br>
> something on the order of:<br>
><br>
><br>
> "force-stack-align"="true"<br>
><br>
><br>
><br>
> versus something like:<br>
><br>
><br>
> target-features="+force-stack-align".<br>
><br>
<br>
</span>Yes.<br>
<span class=""><br>
><br>
> Which I can somewhat agree with if we really want to. I don't know if<br>
> this is better suited toward an actual IR level attribute though?<br>
><br>
> I moved soft-float over to a subtarget feature because it was<br>
> something used to conditionalize initialization for each subtarget.<br>
> RESET_OPTIONS needs to die a horrible death though so I don't think<br>
> we should move this to TargetOptions. If we're going to do something<br>
> then let's just add a target attribute and use that as a lookup. If<br>
> you don't want to use it as a subtarget feature (it's not clear at<br>
> all that it should be I agree), then we should just have it as a<br>
> serializable attribute.<br>
<br>
</span>To be clear, I don't care whether it is a subtarget feature or not. But if it is a subtarget feature, we need a way of doing that in some kind of base class (either in C++ or in TableGen) so that we don't just need to copy-and-paste it into every backend. Adding a particular subtarget feature with a specific name to every target goes beyond justifiable boilerplate.<br>
<br>
And, whatever we do, we really need to be consistent about it. Let's decide on a way forward and unify everything in that direction. We also have direct calls to check attributes in various places (such as 'if (MF.getFunction()->hasFnAttribute("no-frame-pointer-elim-non-leaf"))' in lib/CodeGen/TargetOptionsImpl.cpp) and we could simply add utility functions to MachineFunction if we'd like too.<br>
<br>
 -Hal<br>
<br>
><br>
><br>
> -eric<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div>