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

[Akira Hatanaka via cfe-commits]

On Mon, Aug 17, 2015 at 12:41 PM, Eric Christopher <echristo at gmail.com>
wrote:

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

OK, a subtarget feature should be used if there is a variable of subtarget
that needs initialization. force-align-stack doesn't seem to need a
variable in subtarget, so it can be a function attribute.


>
>>
>>> 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.
>
>
Could you give some examples?

So for a function attribute like "force-align-stack" (can be either an enum
or a string, but note that there is no "false" or "true" value as there is
no need to make it a tri-state), what would the new less verbose
representation look like and how would it improve the way function
attributes are handled?


> -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/eec5a078/attachment-0001.html>