[PATCH] D112142: [InstCombine][ConstantFolding] Make ConstantFoldLoadThroughBitcast TypeSize-aware

[Paul Walker via Phabricator via llvm-commits]

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