[llvm-dev] Improving LLVM string attributes

[David Blaikie via llvm-dev]

Took a quick glance at the review, but didn't look like it answered this
question:

[I] wonder if much/most of the benefit might be gained by moving some
"standard" free form attributes into named/non-free-form attributes in the
first place?

I think /maybe/ the answer to that question is that the standard attributes
don't support values, maybe (I don't know if this is true, I'm
speculating)? So if that's true - then I see there are standard keys that
are currently strings that maybe we could support better by providing some
way to have builtin (non-string key) attributes with values (maybe only
strings still, for now)?

Maybe that would mostly address the perf issue while improving type safety,
etc?

On Mon, Nov 22, 2021 at 1:36 PM Serge Guelton <sguelton at redhat.com> wrote:

> That's all valid points. I submitted a simplified and hopefully easier to
> review version in https://reviews.llvm.org/D114394
>
> On Wed, Nov 17, 2021 at 10:11 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> Might benefit from a separate copy of the review with only the API
>> implementation changes & not all the cleanup/fixes (maybe like one file
>> of/few examples of fixes) - so it's easier to find the implementation/core
>> of the change.
>>
>> I'm modestly in favor - though wonder if much/most of the benefit might
>> be gained by moving some "standard" free form attributes into
>> named/non-free-form attributes in the first place?
>>
>> On Wed, Nov 17, 2021 at 8:10 AM Serge Guelton via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> Hi folks,
>>>
>>> LLVM handles freeform attributes through string attributes
>>> ("key"="value" in the
>>> textual IR). Although it's freeform, some of these attributes are
>>> relatively
>>> standard and widely used (e.g. "target-cpu" or "target-features").
>>>
>>> However, there's no central place where these attributes are either
>>> mentionned,
>>> normalized etc.
>>>
>>> In an effort to improve this siutation, I've submitted
>>>
>>>     https://reviews.llvm.org/D114082
>>>
>>> Which is still a WIP by some aspects, but it compiles and passes
>>> validation on
>>> X86. The key aspects of it are that:
>>>
>>> 1. common attributes get a private symbol to represent them. For instance
>>>
>>>         OutlinedFn->addFnAttr("omp_target_num_teams",
>>> std::to_string(DefaultValTeams));
>>>
>>>    becomes
>>>
>>>         OutlinedFn->addFnAttr(llvm::OmpTargetNumTeamsAttr,
>>> std::to_string(DefaultValTeams));
>>>
>>> 2. free forms attributes are still buildable through a specific factory
>>>
>>>         Fn->addFnAttr(Out.str());
>>>
>>>    becomes
>>>
>>>         Fn->addFnAttr(llvm::AttributeKey::Create(Out.str()));
>>>
>>> I see several advantage in that approach:
>>>
>>> - it avoids typo in common attributes
>>>
>>> - it associates a type to Attributes key (currently named AttributeKey),
>>> which
>>>   unlocks more potential optimization
>>>
>>> - (NIY) one could document the meaning of these attributes alongside
>>> their
>>>   definition, instead of existing mess ;-)
>>>
>>> As a specific optimization, I already made most AttributeKey be
>>> constexpr and have
>>> them store their hash, which makes this implementation faster than
>>> existing
>>> implementation: when checking if an attribute is available, no hash
>>> recomputation is involved.
>>>
>>> /extra note 0/ no change to the IR, that's only a higher level
>>> abstraction over
>>> existing representation. And free-form attributes are still supported,
>>> without
>>> much overhead.
>>>
>>> /extra note 1/ as a side effect, I get a ~1% compile time speedup when
>>> compiling large
>>> file in -O0 with the patch above.
>>>
>>> So yeah, how do you feel about that?
>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211122/47b58b12/attachment.html>