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 <hfin...@anl.gov> wrote: > ----- 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
