This is an archive of the discontinued LLVM Phabricator instance.

[mlir][flang] Convert TBAA metadata to an attribute representation
ClosedPublic

Authored by zero9178 on Jul 17 2023, 4:59 AM.

Details

Summary

The current representation of TBAA is the very last in-tree user of the llvm.metadata operation.
Using ops to model metadata has a few disadvantages:

  • Building a graph has to be done through some weakly typed indirection mechanism such as SymbolRefAttr
  • Creating the metadata has to be done through a builder within a metadata op.
  • It is not multithreading safe as operation insertion into the same block is not thread-safe

This patch therefore converts TBAA metadata into an attribute representation, in a similar manner as it has been done for alias groups and access groups in previous patches.

This additionally has the large benefit of giving us more "correctness by construction" as it makes things like cycles in a TBAA graph, or references to an incorrectly typed metadata node impossible.

Diff Detail

Event Timeline

zero9178 created this revision.Jul 17 2023, 4:59 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Jul 17 2023, 4:59 AM

Very nice! Attributes make things a lot simpler.

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
720–721

Would it make sense to use an array of structs instead of two arrays for member and offests?

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

maybe it is worth trying to grep the relevant files for symbol since I suspect there are still some leftover comments talking about metadata operations and symbols?

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
285–288

ultra nit: ...

mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
112

ultra nit: missing newline :).

118

nit: Can you add a doc comment maybe using the format comment from below. Same would be great for printTBAAMembers!

mlir/lib/Dialect/LLVMIR/IR/LLVMInterfaces.cpp
20

ultra nit: I think I slightly prefer AttrT to make clear it is a type.

52

Would it make sense to perform the same check for the alias and noalias scopes as well now that we have the helper?

53

It seems like this was only verified for the root node before. Is this change intentional?

mlir/lib/Target/LLVMIR/ModuleImport.cpp
265–270
330–349

I wonder if we should drop this check since it is verified afterwards?

335

nit: childNode or just node?

380
mlir/test/Dialect/LLVMIR/tbaa-roundtrip.mlir
2–3

nit: Can we now remove all this modules since all of them share the same metadata.

mlir/test/Target/LLVMIR/tbaa.mlir
2–3

nit: removing the modules may work here as well?

zero9178 updated this revision to Diff 541440.Jul 18 2023, 4:03 AM
zero9178 marked 14 inline comments as done.

Address review comments:

  • Fix nits
  • Change TypeDescriptorAttr to use an array of struct representation
mlir/include/mlir/Target/LLVMIR/ModuleImport.h
181

I have gone through and grepped through the files again and hope I caught them all this time

mlir/lib/Dialect/LLVMIR/IR/LLVMAttrs.cpp
118

I've converted these to to public methods with doc comments in the header as part of changing TBAATypeDescriptorAttr to have an array of structs representation.

mlir/lib/Dialect/LLVMIR/IR/LLVMInterfaces.cpp
53

I have checked the LLVM language reference again and found the following:

The root node of a TBAA type hierarchy is an MDNode with 0 operands or with exactly one MDString operand.

Scalar type descriptors are represented as an MDNode s with two operands. The first operand is an MDString denoting the name of the struct type. LLVM does not assign meaning to the value of this operand, it only cares about it being an MDString.

https://llvm.org/docs/LangRef.html#representation

I think it therefore makes the most sense to just make the string attribute optional in the TBAA root and not verify the contents of the strings at all if present.

gysit added inline comments.Jul 18 2023, 4:56 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMInterfaces.td
227 ↗(On Diff #541440)

Instead of the interface we may want to use a common base class as done for the debug metadata attributes (DINodeAttr or similar?).

mlir/lib/Dialect/LLVMIR/IR/LLVMInterfaces.cpp
50–53
55–58
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
1207
zero9178 updated this revision to Diff 541472.Jul 18 2023, 5:30 AM
zero9178 marked 4 inline comments as done.
  • Address Review comments
  • Turn TBAAMemberAttr into an attribute to let ODS auto generate a lot of the code
gysit accepted this revision.Jul 18 2023, 6:58 AM

Very nice simplification!

LGTM

This revision is now accepted and ready to land.Jul 18 2023, 6:58 AM
zero9178 updated this revision to Diff 541852.Jul 18 2023, 11:53 PM

Don't forget about updating flang too