Page MenuHomePhabricator

[mlir][llvm] Update insertion point handling in LLVM import.
ClosedPublic

Authored by gysit on Oct 25 2022, 6:23 AM.

Details

Summary

Insert constants and globals in order by maintaining the position
of the constant and global inserted last. Update the tests
to reflect the updated insertion order. Also make sure functions
are always inserted at the end of the module instead of at
the second last position and delete a spurious function in
the intrinsic.ll that seems to exist to avoid the first
function under test ends up at the end of the module.

Diff Detail

Event Timeline

gysit created this revision.Oct 25 2022, 6:23 AM
Herald added a project: Restricted Project. · View Herald Transcript
gysit requested review of this revision.Oct 25 2022, 6:23 AM
ftynse added inline comments.Oct 26 2022, 4:42 AM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
433

Naming nit: this returns a block rather than the insertion point, which is a combination a of a block and an iteration in it.

Conceptual nit: by doing this, we will always insert constants in the inverse order of their encounter. This makes the IR surprising to read, but isn't really a functional change. Some ideas for handling it in the future: can't we maintain the Operation * of the last inserted constant and use that to set the insertion point after it?

443–446

Similar as above: can't we just maintain the Operation * of the first function and set the insertion point before it for globals to avoid iterating over the module?

gysit added inline comments.Oct 26 2022, 4:48 AM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
433

I agree the inverse order is problematic and will try to implement a variant with Operation *. Alternatively, we may also have multiple builders?

ftynse added inline comments.Oct 26 2022, 4:50 AM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
433

We may have multiple builders as long as we carefully propagate their extra state such as listeners, although it may not be a concern right now. IMO just setting the insertion point is simple and more future-proof.

gysit added inline comments.Oct 26 2022, 4:53 AM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
433

Sounds good!

gysit updated this revision to Diff 473176.Nov 4 2022, 2:35 AM

Address comments by tracking the insertion points using Operation* / Block*.

gysit edited the summary of this revision. (Show Details)Nov 4 2022, 2:36 AM
ftynse accepted this revision.Nov 7 2022, 5:55 PM
This revision is now accepted and ready to land.Nov 7 2022, 5:55 PM