[PATCH] D114082: [WIP] Normalize String Attributes

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D114082#3149631 , @rnk wrote:

>> it is possible to list all internal keys in one place
>
> I think one of the original goals of adding support for string attributes 
> (maybe @void will give some input) was specifically to avoid having one giant 
> list with all the attributes supported by all the backends. String attributes 
> were initially called "target dependent attributes", or something like that. 
> I think we had a `td_attrs` accessor at some point. There was this idea that 
> LLVM should not know about x86-specific attributes. There should be some kind 
> of indirection and delegation to the target backend. LLVM has not always 
> succeeded in following this vision, see for example the target-specific 
> intrinsic enums, which were one giant enum until D71320 
> .
>
> I think, even if LLVM IR has to know all of the target-specific internally 
> used attributes, we can try to honor that target independent vision by having 
> separate headers for different target specific attributes. Does that sound 
> reasonable? It's at least consistent with how we handle intrinsics 
> (IntrinsicsX86.h, IntrinsicsAArch64.h etc).

The sub-review I proposed in https://reviews.llvm.org/D114394 does not 
introduce any list, so that should be a decent first step. As a second step, 
many passes already register their attribute in some local header and I can 
just generalize that: no big list but some local explicit definition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114082: [WIP] Normalize String Attributes

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: void, rnk.
rnk added a comment.

> it is possible to list all internal keys in one place

I think one of the original goals of adding support for string attributes 
(maybe @void will give some input) was specifically to avoid having one giant 
list with all the attributes supported by all the backends. String attributes 
were initially called "target dependent attributes", or something like that. I 
think we had a `td_attrs` accessor at some point. There was this idea that LLVM 
should not know about x86-specific attributes. There should be some kind of 
indirection and delegation to the target backend. LLVM has not always succeeded 
in following this vision, see for example the target-specific intrinsic enums, 
which were one giant enum until D71320 .

I think, even if LLVM IR has to know all of the target-specific internally used 
attributes, we can try to honor that target independent vision by having 
separate headers for different target specific attributes. Does that sound 
reasonable? It's at least consistent with how we handle intrinsics 
(IntrinsicsX86.h, IntrinsicsAArch64.h etc).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114082: [WIP] Normalize String Attributes

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

This diff is indeed very large. I reworked it in 
https://reviews.llvm.org/D114394 to capture the essence of it, on which we 
could then rebase this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114082: [WIP] Normalize String Attributes

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

The patch is pretty large, any chance you could split in two? First the change 
introducing the AttributeKey and changing the API, and then updating all the 
callees separately as a follow up?




Comment at: llvm/include/llvm/IR/Attributes.h:84
+  }
+};
 

This whole code deserves documentation I think.



Comment at: llvm/lib/IR/Attributes.cpp:125
   FoldingSetNodeID ID;
-  ID.AddString(Kind);
+  ID.AddString(Kind.value());
   if (!Val.empty()) ID.AddString(Val);

Seems like this method does not use anything than the `StringRef` inside the 
Kind, why change it to take an `AttributeKey`? Won't you eagerly compute the 
hash even if not needed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114082: [WIP] Normalize String Attributes

[Yilong Guo via Phabricator via cfe-commits]

Nuu added a comment.

Thanks for this. I planned something similar to turn string literal typo errors 
into compile errors as well.
The typos are really hard to identify :-P

Some suggestions on using tablegen.




Comment at: llvm/include/llvm/IR/Attributes.h:977-1133
+constexpr llvm::AttributeKey
+AmdgpuFlatWorkGroupSizeAttr("amdgpu-flat-work-group-size");
+constexpr llvm::AttributeKey AmdgpuIeeeAttr("amdgpu-ieee");
+constexpr llvm::AttributeKey
+AmdgpuImplicitargNumBytesAttr("amdgpu-implicitarg-num-bytes");
+constexpr llvm::AttributeKey AmdgpuNumSgprAttr("amdgpu-num-sgpr");
+constexpr llvm::AttributeKey AmdgpuNumVgprAttr("amdgpu-num-vgpr");

I think we can simplify these definitions via "Attributes.inc", by adding one 
more class tablegen class and defining the symbolic -> display name mapping in 
"Attributes.td".


```
// llvm/utils/TableGen/Attributes.cpp emitTargetsIndependentNames()
Emit({"EnumAttr", "TypeAttr", "IntAttr"}, "ATTRIBUTE_ENUM");
Emit({"StrBoolAttr"}, "ATTRIBUTE_STRBOOL");
Emit({"StrKeyAttr"}, "ATTRIBUTE_KEY");
```



Comment at: llvm/include/llvm/IR/Attributes.td:43
 class StrBoolAttr : Attr;
 
 /// Target-independent enum attributes.

Define a dedicated tablegen class for AttributeKey generation.



Comment at: llvm/include/llvm/IR/Attributes.td:303
 def UseSampleProfile : StrBoolAttr<"use-sample-profile">;
 
 class CompatRule {

Map symbolic names with string literals.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114082: [WIP] Normalize String Attributes

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

This is a WIP, to start the discussion. I've only turned on the X86 backend. 
Otherwise should be functional.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114082/new/

https://reviews.llvm.org/D114082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits