[PATCH] D114394: Compile-time computation of string attribute hashes

[serge via Phabricator via llvm-commits]

serge-sans-paille marked an inline comment as done.
serge-sans-paille added inline comments.


================
Comment at: llvm/include/llvm/IR/Attributes.h:79
+  bool operator<(AttributeKey const &other) const {
+    return strcmp(value_, other.value_) < 0;
+  }
----------------
JonChesterfield wrote:
> Could order by size first here, then strncmp on equal sizes
I tend to agree, but that would change how attributes are pretty-printed, so I'd rather keep that for another review.


================
Comment at: llvm/lib/IR/AttributeImpl.h:214
 
-  DenseMap<StringRef, Attribute> StringAttrs;
+  std::unordered_map<AttributeKey, Attribute> StringAttrs;
 
----------------
MaskRay wrote:
> std::unordered_map is inefficient. Why is this change?
Because in the specific case of `AttrBuilder`, `std::unordered_map` is actually more efficient. `AttrBuilder` usually only handles a few elements, I even tried an implementation based on a sorted array and it was better than other alternatives in my benchmarks. But that's for another PR.


================
Comment at: llvm/lib/IR/Attributes.cpp:125
   FoldingSetNodeID ID;
-  ID.AddString(Kind);
+  ID.AddString(Kind.value());
   if (!Val.empty()) ID.AddString(Val);
----------------
mehdi_amini wrote:
> Carrying over my comment from the original revision: it seem that you're ever only using the StringRef from the Kind in this function.
> If so changing the API to use an AttributeKey seems pessimizing to me?
`Kind` is actually also used as a parameter of `StringAttributeImpl` which has been updated to store an AttributeKey, so that would just move the AttributeKey creation from the caller to the internal implementation.
And I'd rather have an homogeneous API alongside all string Attribute creation.


================
Comment at: llvm/lib/IR/Attributes.cpp:862
+  auto Where = StringAttrs.find(Kind);
+  return Where != StringAttrs.end() ? Where->second : Attribute();
 }
----------------
MaskRay wrote:
> `lookup`
Not for `std::unordered_map` :-/


================
Comment at: llvm/lib/IR/Attributes.cpp:2054
+  auto &AttributeKeys = pImpl->AttributeKeys;
+  auto Where = AttributeKeys.find(s);
+  if (Where == AttributeKeys.end()) {
----------------
MaskRay wrote:
> If you change `AttributeKeys` to `DenseMap<CachedHashStringRef, ...>`, you can construct a `CachedHashStringRef` and avoid a hash computation in `AttributeKey Key(s);`
I was looking for something along these lines, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1531
 static StringRef getDeoptLowering(CallBase *Call) {
-  const char *DeoptLowering = "deopt-lowering";
+  const char DeoptLowering[] = "deopt-lowering";
   if (Call->hasFnAttr(DeoptLowering)) {
----------------
MaskRay wrote:
> This can be committed separately.
Actually it cannot, because the `AttributeKey` constructor accepts a `char const (&) [N]` in order to get a compile-time size, and `const char*` doesn match this signature.


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

https://reviews.llvm.org/D114394