[PATCH] D11815: Pass subtarget feature "force-align-stack"

[Hal Finkel via cfe-commits]

----- Original Message -----
> From: "Eric Christopher" <echristo at gmail.com>
> To: reviews+D11815+public+324fadcdaae02561 at reviews.llvm.org, ahatanak at gmail.com, dexonsmith at apple.com,
> hfinkel at anl.gov
> Cc: cfe-commits at lists.llvm.org
> Sent: Thursday, August 13, 2015 7:39:53 PM
> Subject: Re: [PATCH] D11815: Pass subtarget feature "force-align-stack"
> 
> On Thu, Aug 13, 2015 at 5:16 PM hfinkel at anl.gov < hfinkel at anl.gov >
> wrote:
> 
> 
> hfinkel added a comment.
> 
> In http://reviews.llvm.org/D11815#224169 , @echristo wrote:
> 
> > No, RESET_OPTION isn't the right way to do this either. For exactly
> > this sort of reason. You can't actually represent all of the code
> > and options this way in the IR. If you can't do that then it's a
> > non-starter.
> 
> 
> Can you please be more specific, what is it that we cannot represent?
> TargetOptions represents the target-generic code-generation options
> for the current function (where the "current function" bit works
> because the options get reset based on the current function
> attributes). That's the design we have now. And sticking with that
> pattern, even if we're going to change the overall scheme later, is
> better than the code duplication and inconsistency proposed here.
> 
> Apologies, I'm really resistant to more things being used in
> TargetOptions and I was (perhaps mistakenly) under the impression
> that you wanted to move it to TargetOptions without an IR
> serialization. We need all options to have that sort of
> serialization right? :)

Absolutely, they all need function-level serialization for LTO to work. We're definitely both on the same page there :)

> In this case it's for the -mstackrealign
> option and we need to keep that if it's going to work for separate
> compilation.
> 
> 
> I'm guessing from the comment here that you're talking about
> something on the order of:
> 
> 
> "force-stack-align"="true"
> 
> 
> 
> versus something like:
> 
> 
> target-features="+force-stack-align".
> 

Yes.

> 
> Which I can somewhat agree with if we really want to. I don't know if
> this is better suited toward an actual IR level attribute though?
> 
> I moved soft-float over to a subtarget feature because it was
> something used to conditionalize initialization for each subtarget.
> RESET_OPTIONS needs to die a horrible death though so I don't think
> we should move this to TargetOptions. If we're going to do something
> then let's just add a target attribute and use that as a lookup. If
> you don't want to use it as a subtarget feature (it's not clear at
> all that it should be I agree), then we should just have it as a
> serializable attribute.

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.

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.

 -Hal

> 
> 
> -eric

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory