This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVM] Convert alias metadata to using attributes instead of ops
ClosedPublic

Authored by zero9178 on Jul 13 2023, 12:47 AM.

Details

Summary

Using MLIR attributes instead of metadata has many advantages:

  • No indirection: Attributes can simply refer to each other seemlessly without having to use the indirection of SymbolRefAttr. This also gives us correctness by construction in a lot of places as well
  • Multithreading save: The Attribute infrastructure gives us thread-safety for free. Creating operations and inserting them into a block is not thread-safe. This is a major use case for e.g. the inliner in MLIR which runs in parallel
  • Easier to create: There is no need for a builder or a metadata region

This patch therefore does exactly that. It leverages the new distinct attributes to create distinct alias domains and scopes in a deterministic and threadsafe manner.

Diff Detail

Event Timeline

zero9178 created this revision.Jul 13 2023, 12:47 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Jul 13 2023, 12:47 AM
zero9178 updated this revision to Diff 539885.Jul 13 2023, 12:58 AM

remove code leftovers, update doc

zero9178 updated this revision to Diff 540286.Jul 13 2023, 11:38 PM
zero9178 retitled this revision from WIP: [mlir][LLVM] Convert alias metadata to using attributes instead of ops to [mlir][LLVM] Convert alias metadata to using attributes instead of ops.

Rebase and minor cleanups.

Ready for review now.

Nice improvement!

I found some outdated comments that should be updated. Otherwise, the revision looks good!

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
570

ultra nit...

580

Would it make sense to create the create method here?

585

I would drop the dot since that was probably coming from the old metadata operation name?

627

I would probably remove the module to save the indentation.

629

I think this would not even be printed and probably can be removed?

mlir/include/mlir/Target/LLVMIR/ModuleImport.h
154

Converts value to an array of alias scope attributes?

205

nit: the comment should be updated as well to say the function return alias scope attributes.

352–353

nit: the comment should be updated to say the mapping points to alias scope or alias domain attributes?

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
126

nit: alias scope operation -> alias scope attribute here and below?

127

nit: Can we use aliasScopeAttr since it is not a symbol reference anymore (below as well).

mlir/lib/Target/LLVMIR/ModuleImport.cpp
457

nit: alias scope attribute

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1142

nit: I would rename to aliasScopeAttrs here and below.

zero9178 updated this revision to Diff 540315.Jul 14 2023, 1:28 AM
zero9178 marked 11 inline comments as done.

Address review comments

zero9178 updated this revision to Diff 540317.Jul 14 2023, 1:31 AM

Fix syntax in TableGen

gysit accepted this revision.Jul 14 2023, 1:55 AM

LGTM module last nit comments.

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
642

ultra nit :)

mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMIRToLLVMTranslation.cpp
194

ultra nit: one more instance of symbol references and below as well.

mlir/test/Dialect/LLVMIR/invalid.mlir
993

I assume this updates as well as soon as the summary is updated.

This revision is now accepted and ready to land.Jul 14 2023, 1:55 AM
This revision was automatically updated to reflect the committed changes.
zero9178 marked 3 inline comments as done.