This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Require struct indices in LLVM::GEPOp to be constant
ClosedPublic

Authored by ftynse on Jan 6 2022, 10:27 AM.

Details

Summary

Recent commits added a possibility for indices in LLVM dialect GEP operations
to be supplied directly as constant attributes to ensure they remain such until
translation to LLVM IR happens. Make this required for indexing into LLVM
struct types to match LLVM IR requirements, otherwise the translation would
assert on constructing such IR.

For better compatibility with MLIR-style operation construction interface,
allow GEP operations to be constructed programmatically using Values pointing
to known constant operations as struct indices.

Depends On D116758

Diff Detail

Event Timeline

ftynse created this revision.Jan 6 2022, 10:27 AM
ftynse requested review of this revision.Jan 6 2022, 10:27 AM
wsmoses added inline comments.Jan 6 2022, 10:46 AM
mlir/test/Dialect/LLVMIR/invalid.mlir
1239

It would be nice to test for when the constant offset is out of bounds of the struct, but that doesn't have to happen in this PR

ftynse updated this revision to Diff 397994.Jan 6 2022, 2:32 PM
ftynse marked an inline comment as done.

Add extra verification requested in review.

wsmoses accepted this revision.Jan 7 2022, 12:43 AM
This revision is now accepted and ready to land.Jan 7 2022, 12:43 AM
This revision was landed with ongoing or failed builds.Jan 7 2022, 12:56 AM
This revision was automatically updated to reflect the committed changes.

For better compatibility with MLIR-style operation construction interface, allow GEP operations to be constructed programmatically using Values pointing to known constant operations as struct indices.

This kind of API are expensive, but more importantly easy to misused: folks can call into this thinking that they can use any value.
I rather remove this entirely.

ArrayRef<int32_t> structIndices,

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
457

llvm::is_contained?

463

expected

515

I always find easier to read it with llvm:seq:

for (int i : llvm::seq<int>(0, indices.size()) {

518

Please move the gepOp.getStructIndices() outside of the loop (and CSE with below). This is an attribute lookup I believe: not "free".

ftynse marked 4 inline comments as done.Jan 10 2022, 3:41 AM

c44d521b3054b7d8dc923d13fe7723cfd44807c8

This kind of API are expensive, but more importantly easy to misused: folks can call into this thinking that they can use any value.
I rather remove this entirely.

There's an assertion to avoid misuses.

I intend to remove this functionality at some later point after an adaptation period.

c44d521b3054b7d8dc923d13fe7723cfd44807c8

This kind of API are expensive, but more importantly easy to misused: folks can call into this thinking that they can use any value.
I rather remove this entirely.

There's an assertion to avoid misuses.

That's not really the point of the "easy to misuse" argument: the API does not express the intent clearly, and the assertions is only input dependent. You can very well be writing users for this API that don't trip the assert on the test, but will later when deployed in production (where it runs without assert by the way).

I intend to remove this functionality at some later point after an adaptation period.

Thanks!