This is an archive of the discontinued LLVM Phabricator instance.

[LoopLoadElim] Add stores with matching sizes as load-store candidates
ClosedPublic

Authored by jolanta.jensen on Jul 21 2022, 2:29 AM.

Details

Summary

We are not building up a proper list of load-store candidates because
we are throwing away stores where the type don't match the load.
This patch adds stores with matching store sizes as candidates.
Author of the original patch: David Sherwood.

Diff Detail

Event Timeline

jolanta.jensen created this revision.Jul 21 2022, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 2:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jolanta.jensen requested review of this revision.Jul 21 2022, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 2:29 AM
nikic requested changes to this revision.Jul 21 2022, 2:40 AM
nikic added a subscriber: nikic.

Not all types are bitcastable. Please add tests where the load/store are pointer and integer (in which case you need inttoptr or ptrtoint -- and watch out for non-integral address spaces) and one where an aggregate is involved (cannot be bitcast at all).

This revision now requires changes to proceed.Jul 21 2022, 2:40 AM
mgabka added a subscriber: mgabka.Jul 27 2022, 6:40 AM
mgabka added inline comments.
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
216

is it worth to make this work also for vector types? the patch you proposed does not work when there is casting between scalar and vector type of the same size for example i32 and <2 x i16>, would it be useful to extend it to cover such cases?

david-arm added inline comments.Jul 28 2022, 1:06 AM
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
216

I don't think you can get such bitcasts, but also I think it's not even safe for big endian due to the different memory layout.

  1. Guarding against non-valid bitcasts.
  2. Load-store forwarding works with vector types as well.
  3. Added a test for load-store forwarding with vector types.
  4. Added tests confirming bitcasting vectors won't corrupt the values stored.

Not all types are bitcastable. Please add tests where the load/store are pointer and integer (in which case you need inttoptr or ptrtoint -- and watch out for non-integral address spaces) and one where an aggregate is involved (cannot be bitcast at all).

@nikic , Thank you for a quick review and apologies for the delay addressing them. I'm now guarding against non-valid bitcasts. For inttoptr or ptrtoint request, f3 in type-mismatch-opaque-ptr.ll has such a case. Do you want more extensive testing than that?

mgabka added inline comments.Aug 9 2022, 2:30 AM
llvm/test/Transforms/EarlyCSE/vector_bitcasting_be.ll
1 ↗(On Diff #450785)

I think a good idea would be to run directly only the passes you actually want to test instead of using O3

9 ↗(On Diff #450785)

can you just pass val1 as argument to this function and remove the alloca?

same request about f2 function and the other file with tests that you added.

nikic added inline comments.Aug 9 2022, 3:13 AM
llvm/test/Transforms/EarlyCSE/vector_bitcasting_be.ll
1 ↗(On Diff #450785)

The test should also be in the LoopLoadElim directory rather than EarlyCSE, and use update_test_checks.py.

llvm/test/Transforms/LoopLoadElim/type-mismatch.ll
144

Eh, while that happens to work out okay here, please do replace all your getPrimitiveSizeInBits() calls with getTypeSizeInBits() on DataLayout.

david-arm added inline comments.Aug 10 2022, 7:36 AM
llvm/test/Transforms/LoopLoadElim/type-mismatch.ll
147

Hi @jolanta.jensen, I think it would be good if you could pre-commit these tests separately in a different patch, then rebase your changes on top. That way we can see the change in output and it makes it easier to see what effect the patch has.

Also, in the same pre-commit patch it would be good if you could generate the CHECK lines with the update_test_checks.py script because I find it a bit hard to see what's going on with partial CHECK lines, mixed between lines of IR.

Thanks!

  1. Correcting errand assumption that the size of a pointer is 0.
  2. Removing vector bitcasting tests as they were adding more confusion than value.
  3. Regenerating CHECK lines with update_test_checks.py script.

Allowing integer-sized pointers to be forwarded.

Also updating the patch to be based on D132239 and addressing review comments.

jolanta.jensen marked 2 inline comments as done.Aug 24 2022, 8:23 AM
nikic added inline comments.Aug 29 2022, 3:44 AM
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
232

I believe the condition you are looking for is CastInst::isBitOrNoopPointerCastable(StoreTy, LoadTy, DL). That should ensure that one of bitcast, inttoptr or ptrtoint is legal for these types.

474

In this whole block, I believe you are looking for CastInst::CreateBitOrPointerCast().

Simplified the code in accordance with review comments.

jolanta.jensen marked 2 inline comments as done.Aug 31 2022, 7:52 AM
nikic added a comment.Aug 31 2022, 8:20 AM

Looks about right, some nits

llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
115–116

Move this DL variable before the assert and reuse.

220

Just the isBitOrNoopPointerCastable check is enough, it implies that the sizes are the same as well.

444

Move this declaration lower, before the assignment.

454

Storing DataLayout in a variable would make this more compact.

Code clean up after review comments.

jolanta.jensen marked 4 inline comments as done.Aug 31 2022, 10:01 AM
fhahn accepted this revision.Aug 31 2022, 10:15 AM
fhahn added a subscriber: fhahn.

LGTM, thanks! Some additional suggestions inline.

llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
218

nit: reflow comment, also now that you use isBitOrNoopPointerCastable it would probably be easier to say that stored values are bit/pointer-castable.

445

nit: getStoredValue() would be more explicit than getOperand(0).

446

Looks like this is only used by the assert. If that's the case, you'll need (void)DL; to avoid unused variable warning in release builds.

452

nit: stray new line?

458

Nit: could change to

Value *StoreValue = Cand.Store->getOperand(0);` 
if (...)
   StoreValue = CastInst::CreateBitOrPointerCast(StoreValue, LoadType,
                                       "store_forward_cast", Cand.Store);

Yet another clean up after review comments.

jolanta.jensen marked 4 inline comments as done.Sep 1 2022, 7:22 AM
jolanta.jensen added inline comments.
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
445

I did not find getStoredValue() in StoreInst class. I changed to getValueOperand() instead.

458

Also replaced getOperand(0) with getValueOperand().

nikic accepted this revision.Sep 1 2022, 7:37 AM

LG

llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
101

Needs a (void)StoreType, or move the call into the assert.

This revision is now accepted and ready to land.Sep 1 2022, 7:37 AM
jolanta.jensen marked 2 inline comments as done.

Addressing one more review comment.

jolanta.jensen marked an inline comment as done.Sep 1 2022, 8:31 AM