This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Support opaque types in LLVM IR -> MLIR translation
ClosedPublic

Authored by ftynse on Apr 14 2022, 6:15 AM.

Details

Summary

LLVM IR is moving towards adoption of opaque pointer types. These require extra
information to be passed when constructing some operations, in particular GEP
and Alloca. Adapt the builders of said operations and modify the translation
code to handle both opaque and non-opaque pointers.

This incidentally adds the translation for Alloca alignment and fixes the translation
of struct-related GEP indices that must be constant.

Diff Detail

Event Timeline

ftynse created this revision.Apr 14 2022, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 6:15 AM
ftynse requested review of this revision.Apr 14 2022, 6:15 AM
ftynse edited the summary of this revision. (Show Details)Apr 14 2022, 6:16 AM
wsmoses added inline comments.Apr 14 2022, 9:48 AM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
876

See comment in test, but just because an index is a constant int doesn't mean that it's a struct or array offset (though those require a constant integer)

mlir/test/Target/LLVMIR/import.ll
26

Does this actually make sense to have as the constant attribute, since its not a struct or array index?

ftynse marked 2 inline comments as done.Apr 14 2022, 2:19 PM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
876

The relationship is not a bijection. Struct indices must be static constants, other indices may or may not be.

mlir/test/Target/LLVMIR/import.ll
26

It shouldn't be an issue.

wsmoses accepted this revision.Apr 14 2022, 2:26 PM
wsmoses added inline comments.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
876

I suppose that's fair. That said, I do feel like it is beneficial to not promote non struct/array constants, if possible (and even if not semantically required).

Specifically, I recall the reason we introduced the attribute is to prevent inadvertant CSE where two geps with constant bounds from different branch predecessors were turned into one GEP with a new block argument. This would be illegal since LLVM needs the fixed offset. For non struct/array constants this wasn't needed as an attribute.

If there's no other reasons for the GEP to have a constant that cropped up since, it would be nice to preserve the offset as an op specifically to enable those optimizations, since we know it's legal.

I'm okay (though not over-the-moon) with the change in the PR, but since it is changing the semantics of importing GEP, it may be worthwhile considering the implications.

This revision is now accepted and ready to land.Apr 14 2022, 2:26 PM
ftynse marked 3 inline comments as done.Apr 15 2022, 2:59 AM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
876

Fair enough.

ftynse updated this revision to Diff 423061.Apr 15 2022, 3:01 AM
ftynse marked an inline comment as done.

Address review.

This revision was landed with ongoing or failed builds.Apr 15 2022, 8:51 AM
This revision was automatically updated to reflect the committed changes.