This is an archive of the discontinued LLVM Phabricator instance.

[SymbolRefAttr] Revise SymbolRefAttr to hold a StringAttr.
ClosedPublic

Authored by lattner on Aug 29 2021, 2:22 PM.

Details

Summary

SymbolRefAttr is fundamentally a base string plus a sequence
of nested references. Instead of storing the string data as
a copies StringRef, store it as an already-uniqued StringAttr.

This makes a lot of things simpler and more efficient because:

  1. references to the symbol are already stored as StringAttr's: there is no need to copy the string data into MLIRContext multiple times.
  2. This allows pointer comparisons instead of string comparisons (or redundant uniquing) within SymbolTable.cpp.
  3. This allows SymbolTable to hold a DenseMap instead of a StringMap (which again copies the string data and slows lookup).

This is a moderately invasive patch, so I kept a lot of
compatibility APIs around. It would be nice to explore changing
getName() to return a StringAttr for example (right now you have
to use getNameAttr()), and eliminate things like the StringRef
version of getSymbol.

Diff Detail

Event Timeline

lattner created this revision.Aug 29 2021, 2:22 PM
lattner requested review of this revision.Aug 29 2021, 2:22 PM
rriddle added inline comments.Aug 29 2021, 2:50 PM
mlir/include/mlir/IR/BuiltinAttributes.td
883–894
mlir/include/mlir/IR/SymbolInterfaces.td
178
183
198–210
199
204
mlir/include/mlir/IR/SymbolTable.h
225–228
mlir/lib/Dialect/SPIRV/Linking/ModuleCombiner/ModuleCombiner.cpp
38
mlir/lib/IR/Builders.cpp
220–228

I would remove these completely. We generally don't add builder methods for things that don't need the internal context in some way.

mlir/lib/IR/BuiltinAttributes.cpp
279–284
mlir/lib/IR/SymbolTable.cpp
28

This isn't necessary. Symbol operations already have it cached in there AbstractOperation. Restructuring this code and it's interaction with SymbolOpInterface would fix this.

rriddle accepted this revision.Aug 29 2021, 2:51 PM

I'm slightly hesitant about all of the new StringAttr uniquing, but assuming we don't need to unique something in the common case I suppose it should be fine.

This revision is now accepted and ready to land.Aug 29 2021, 2:51 PM
lattner updated this revision to Diff 369370.Aug 29 2021, 9:54 PM
lattner marked 9 inline comments as done.

Incorporate River's feedback, thanks River!

mlir/lib/IR/Builders.cpp
220–228

The problem is that the StringRef version does arguably belong as part of Builder... and I don't want to have "just" the stringref version and not have the other one next to it.

I'll look into removing both completely in a follow-on patch. Thanks for pointing this out.

mlir/lib/IR/SymbolTable.cpp
28

I don't understand what you're suggesting, but I removed the comment.

The issue here is that getAttrOfType(StringRef) has to turn the StringRef into an Identifier to do the lookup or it does inefficient string compares. Other stuff is similarly doing stuff with getSymbolAttrName() that does similar uniquing. Symbols seem important enough to put one slot into MLIRContext to improve this. If you have another way to improve this, that would be great!

I'm slightly hesitant about all of the new StringAttr uniquing, but assuming we don't need to unique something in the common case I suppose it should be fine.

Understood. Two things:

  1. It is really just _moving_ the uniquing in the normal case, because the StringRef stuff typically ends up uniquing it when forming the symbol anyway.
  2. In lots of cases (particularly internal to the mechanics of the SymbolTable) we already have the attr around, so there isn't new uniquing.

There is definitely the chance to remove more use of the StringRef API (in favor of things that pass around the attrs) of course. I'll see how many of the convenience APIs can be reasonably removed. I only did a superficial pass in this patch because it was getting large as is.

Thank you so much for the quick review of this!

This revision was landed with ongoing or failed builds.Aug 29 2021, 10:01 PM
This revision was automatically updated to reflect the committed changes.

Am I reading the report right? Did this go in with build failures?