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

[Akira Hatanaka via cfe-commits]

On Thu, Aug 13, 2015 at 5:56 PM, Eric Christopher <echristo at gmail.com>
wrote:

>
>> > 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 :)
>>
>>
> Cool.
>
>
>> > 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.
>>
>>
> Yep.
>
>
>> >
>> > 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.
>>
>>
> Agreed. It's one reason the patch had sat for a while (thanks for looking
> btw, it spurred me to a bit of action). I had some patches that added
> generic subtarget features to Target.td for soft float originally and was
> convinced to do the per-target bit. I agree that per-target is insanely
> boilerplate here and we should come up with something else.
>
>

If we aren't going to have generic subtarget features, I think we should
just use function attributes for target independent code-gen options like
force-align-stack.


> 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.
>>
>>
> I'm all about something new here. I've got "use-soft-float"="true"
> autoupgrading to the particular subtarget feature now (IIRC), but these
> kinds of string pair features are a bit odd after a while. Perhaps either a
> generic target-options="stuff" on the function that gets parsed once at
> Function creation time? That seems nice and extensible?
>
>
So, this is about changing the implementation of Attribute or AttributeSet
and convert "attrkind"="attrval" in the IR to something different
internally? Is this supposed to fix some flaws of Attribtue or AttributeSet?


> -eric
>
>
>>  -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/20150817/eea62284/attachment-0001.html>