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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
