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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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).
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp | ||
---|---|---|
220 | 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? |
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp | ||
---|---|---|
220 | 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. |
- Guarding against non-valid bitcasts.
- Load-store forwarding works with vector types as well.
- Added a test for load-store forwarding with vector types.
- Added tests confirming bitcasting vectors won't corrupt the values stored.
@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?
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. |
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. |
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! |
- Correcting errand assumption that the size of a pointer is 0.
- Removing vector bitcasting tests as they were adding more confusion than value.
- 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.
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp | ||
---|---|---|
237 | 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. | |
492 | In this whole block, I believe you are looking for CastInst::CreateBitOrPointerCast(). |
Looks about right, some nits
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp | ||
---|---|---|
119 | Move this DL variable before the assert and reuse. | |
224 | Just the isBitOrNoopPointerCastable check is enough, it implies that the sizes are the same as well. | |
462 | Move this declaration lower, before the assignment. | |
472 | Storing DataLayout in a variable would make this more compact. |
LGTM, thanks! Some additional suggestions inline.
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp | ||
---|---|---|
222 | nit: reflow comment, also now that you use isBitOrNoopPointerCastable it would probably be easier to say that stored values are bit/pointer-castable. | |
463 | nit: getStoredValue() would be more explicit than getOperand(0). | |
464 | 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. | |
470 | nit: stray new line? | |
476 | Nit: could change to Value *StoreValue = Cand.Store->getOperand(0);` if (...) StoreValue = CastInst::CreateBitOrPointerCast(StoreValue, LoadType, "store_forward_cast", Cand.Store); |
LG
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp | ||
---|---|---|
101 | Needs a (void)StoreType, or move the call into the assert. |
Needs a (void)StoreType, or move the call into the assert.