----- Original Message -----
> From: "Eric Christopher" <echri...@gmail.com>
> To: reviews+d11815+public+324fadcdaae02...@reviews.llvm.org,
> ahata...@gmail.com, dexonsm...@apple.com,
> hfin...@anl.gov
> Cc: cfe-commits@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 hfin...@anl.gov < hfin...@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
