This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] add basic type consistency pattern destructuring integer stores
ClosedPublic

Authored by zero9178 on Jul 4 2023, 7:55 AM.

Details

Summary

This is a common pattern produced by clang and similar.
Essentially, it coalesces stores into adjacent integer fields into a single integer store.
This violates our definition of type-consistency that the pass is supposed to enforce and also prevents SROA and mem2reg from eliminating allocas.

This patch fixes that by splitting these stores into multiple stores.
It does so by simply using logical shift rights and truncating the produced value to the size of the field, optionally bitcasting before storing into the field.

The implementation is currently very simple, only working on struct types of a single depth and adjacent fields in that struct, with no padding inbetween. Future work could improve on these once required.

Diff Detail

Event Timeline

zero9178 created this revision.Jul 4 2023, 7:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Jul 4 2023, 7:55 AM
gysit added a comment.Jul 4 2023, 11:52 PM

Thanks!

The logic looks good. I have added a few nit comments.

mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp
314–316

Would it make sense to sort of invert the logic of this function and return std::optional<Type>?

In any case it would be great to add a doc string for all static functions and I would also try to start the function names with a verb. E.g.,

static std::optional<Type> getConsistentType(GEPOp gep);
359

Any chance we could work with ArrayRef<Type> and use drop front / tail to remove the parts that do not map to the store?

393

ultra nit: braces would be needed due to the comment. Same right below.

459

nit: t -> type or at least ty. I would try to avoid single letter variable names.

468

nit: let's use shrOp and gepOp (below) for operations.

470

nit: fieldIntType?

mlir/test/Dialect/LLVMIR/type-consistency.mlir
158

nit: Let's use CST0 and CST32?

229

Would it make sense to add a test with a packed struct of the form {i16, i32, i16}. In this case, we should be able to split the store?

zero9178 updated this revision to Diff 537247.Jul 5 2023, 12:51 AM
zero9178 marked 8 inline comments as done.

Address review comments:

  • Adjust naming nits in C++ and FileCheck
  • Add test for packed struct
  • Rewrite code getting written to fields to work nicely with ArrayRef
  • Other nits
mlir/test/Dialect/LLVMIR/type-consistency.mlir
229

Correct, since no padding would be written to. I added a testcase showcasing it.

gysit accepted this revision.Jul 5 2023, 3:52 AM

Nice improvement!

LGTM modulo some ultra nits.

mlir/lib/Dialect/LLVMIR/Transforms/TypeConsistency.cpp
314–317
358

ultra nit: Can you use three slashes as well and ideally also back ticks ` instead of ' for the variable names.

368

ultra nit: I would drop the newlines at the beginning of the function

403

ultra nit: unsigned int -> unsigned

This revision is now accepted and ready to land.Jul 5 2023, 3:52 AM
zero9178 updated this revision to Diff 537298.Jul 5 2023, 4:56 AM
zero9178 marked 4 inline comments as done.

address review comments

This revision was landed with ongoing or failed builds.Jul 5 2023, 4:57 AM
This revision was automatically updated to reflect the committed changes.