<div dir="ltr">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.<br><br>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?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 17, 2021 at 8:10 AM Serge Guelton via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi folks,<br>
<br>
LLVM handles freeform attributes through string attributes ("key"="value" in the<br>
textual IR). Although it's freeform, some of these attributes are relatively<br>
standard and widely used (e.g. "target-cpu" or "target-features").<br>
<br>
However, there's no central place where these attributes are either mentionned,<br>
normalized etc.<br>
<br>
In an effort to improve this siutation, I've submitted<br>
<br>
    <a href="https://reviews.llvm.org/D114082" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114082</a><br>
<br>
Which is still a WIP by some aspects, but it compiles and passes validation on<br>
X86. The key aspects of it are that:<br>
<br>
1. common attributes get a private symbol to represent them. For instance<br>
<br>
        OutlinedFn->addFnAttr("omp_target_num_teams", std::to_string(DefaultValTeams));<br>
<br>
   becomes<br>
<br>
        OutlinedFn->addFnAttr(llvm::OmpTargetNumTeamsAttr, std::to_string(DefaultValTeams));<br>
<br>
2. free forms attributes are still buildable through a specific factory<br>
<br>
        Fn->addFnAttr(Out.str());<br>
<br>
   becomes<br>
<br>
        Fn->addFnAttr(llvm::AttributeKey::Create(Out.str()));<br>
<br>
I see several advantage in that approach:<br>
<br>
- it avoids typo in common attributes<br>
<br>
- it associates a type to Attributes key (currently named AttributeKey), which<br>
  unlocks more potential optimization<br>
<br>
- (NIY) one could document the meaning of these attributes alongside their<br>
  definition, instead of existing mess ;-)<br>
<br>
As a specific optimization, I already made most AttributeKey be constexpr and have<br>
them store their hash, which makes this implementation faster than existing<br>
implementation: when checking if an attribute is available, no hash<br>
recomputation is involved.<br>
<br>
/extra note 0/ no change to the IR, that's only a higher level abstraction over<br>
existing representation. And free-form attributes are still supported, without<br>
much overhead.<br>
<br>
/extra note 1/ as a side effect, I get a ~1% compile time speedup when compiling large<br>
file in -O0 with the patch above.<br>
<br>
So yeah, how do you feel about that?<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>