Page MenuHomePhabricator

[MLIR][LLVM] Add llvm.mlir.global_ctors/dtors and translation support
ClosedPublic

Authored by bondhugula on Oct 26 2021, 3:43 AM.

Details

Summary

Add llvm.mlir.global_ctors and global_dtors ops and their translation
support to LLVM global_ctors/global_dtors global variables.

Diff Detail

Event Timeline

bondhugula created this revision.Oct 26 2021, 3:43 AM
bondhugula requested review of this revision.Oct 26 2021, 3:43 AM

Nice, thanks!

Do you have an idea of what would it take to also model the data field?

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1165

Loaded?

1173

The syntax in examples does not correspond to the assembly format below.

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

How does this behave when the same symbol is listed more the once in the constructor list? Should we add a check?

bondhugula added a comment.EditedOct 26 2021, 4:21 AM

Nice, thanks!

Do you have an idea of what would it take to also model the data field?

The data field in the corresponding LLVM globals has too low-level semantics and surrounding context attached to it to be able to meaningfully describe in MLIR at this stage -- it's connected to object file sections and linkers, and is sensitive to binary formats.

bondhugula marked 2 inline comments as done.Oct 26 2021, 4:32 AM
bondhugula added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
639

The LLVM global ctors/dtors list itself doesn't appear to care about duplicates. They are just sorted based on priority and called (so potentially multiple times based on a quick read of the LLVM code). But I actually missed adding verifiers. We should have been checking for example that the sizes of array attributes matched.

Address some review comments.

ftynse added inline comments.Oct 26 2021, 12:51 PM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
639

I see, thanks! Let's please have verifiers for the equal number of elements in the arrays and also for symbol references referring to symbols that are actually defined as functions.

bondhugula updated this revision to Diff 382521.EditedOct 26 2021, 9:28 PM

Add verifiers to check symbol and priority list sizes, and for valid
defined functions. Switch priority to i32.

bondhugula marked 2 inline comments as done.Oct 26 2021, 9:29 PM

Indent and update doc.

Update op doc.

Drop unnecessary header.

Gentle ping @ftynse - this one is now ready for review.

Move static method above.

ftynse accepted this revision.Oct 27 2021, 11:40 PM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1165

Ultra-nit: sometimes there are two spaces between sentences and sometimes there is only one, I don't mind either, but let's be consistent.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1647 ↗(On Diff #382588)

Nit: explicit this-> looks unnecessary here.

This revision is now accepted and ready to land.Oct 27 2021, 11:40 PM
bondhugula marked 2 inline comments as done.

Address remaining review comments.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1165

Fixed.