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

[Eric Christopher via cfe-commits]

On Mon, Aug 17, 2015 at 11:57 AM Akira Hatanaka <ahatanak at gmail.com> wrote:

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

I've been using subtarget features for anything necessary to initialize the
subtarget. Notice soft float for example.


>
>
>> 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?
>

No, thinking about representing certain sets of features as "attributes on
functions" that are perhaps a bit more free form, but not as ... verbose as
the current implementation.

-eric


>
>
>> -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/57b50ff5/attachment.html>