[PATCH] D112142: [InstCombine][ConstantFolding] Make ConstantFoldLoadThroughBitcast TypeSize-aware
Paul Walker via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 20 10:43:53 PDT 2021
paulwalker-arm added inline comments.
================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:357
+ TypeSize SrcSize = DL.getTypeSizeInBits(SrcTy);
+ if (TypeSize::isKnownLT(SrcSize, DestSize))
return nullptr;
----------------
You cannot use TypeSize in this way. Essentially TypeSize can only prove a positive not a negative.
This code block is effectively saying "the following code is only safe when SrcSize >= DestSize and you must exit when this is not true". Whereas you've changed it to be "if and only if I can prove SrcSize < DestSize shall I exit".
So I believe the correct change is rather
```
if (!TypeSize::isKnownGE(SrcSize, DestSize))`
```
================
Comment at: llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll:6
+; CHECK-LABEL: @constprop_load_bitcast(
+; CHECK-NEXT: ret <8 x i8> zeroinitializer
+;
----------------
david-arm wrote:
> I guess this fold is happening because effectively i1 is being promoted to i8? Otherwise the minimum size of <vscale x 16 x i1> would be just 2 bytes, i.e. less than than <8 x i8>.
This is likely due to the issue above but at the same time the test is relying on undefined behaviour. You either want to use a smaller fixed length vector or perhaps add a `vscale_range` so the load does overlap the store.
Can you also add a negative test for the case where the load & store must remain. I guess if you want to tick all the boxes you could also add a test where the store remains but the load is removed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112142/new/
https://reviews.llvm.org/D112142
More information about the llvm-commits
mailing list