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

[Akira Hatanaka via cfe-commits]

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).

On Thu, Aug 13, 2015 at 5:52 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150813/890c69ef/attachment.html>