[InstCombine][ConstantFolding] Make ConstantFoldLoadThroughBitcast TypeSize-aware

[peterwaller-arm]

Summary

The newly added test previously caused the compiler to fail an
assertion. It looks like a strightforward TypeSize upgrade.

[david-arm]

Hi @peterwaller-arm, the patch looks good to me! I wonder if it's worth renaming the test to vscale_load.ll so that it's less specific? That way we can potentially more load related fold tests here in future as well.

llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll

6 ↗

(On Diff #380964)

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>.

[paulwalker-arm]

llvm/lib/Analysis/ConstantFolding.cpp

357

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))`

llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll

6 ↗

(On Diff #380964)

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.

[peterwaller-arm]

• Fix typesize query to use !isKnownGE.
• Add a negative test.

[peterwaller-arm]

llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll

6 ↗

(On Diff #380964)

Right. I've added a negative test and fixed up the size query, thanks both for your attention to detail!

I considered your comment about vscale_range but:

1. I don't see a straightforward route to get at it without propagating the function attributes through the constant folding chain. Maybe that's the right thing to do? Or is there a way to take Constant* and a Type*, and ask "what vscale_range is in effect?". I'm guessing not since these aren't in principle bound to functions. I don't want to fiddle with the function signature without a positive steer from someone that it's the right thing to do.

2. What is the best way to make isKnown* queries in the presence of a vscale_range attribute?

[paulwalker-arm]

My vscale_range comment is not concerning the implementation but rather about ensuring the ll tests do not contain potentially undefined behaviour. You can ignore this because I've commented below with an alternative that is simpler.

llvm/test/Transforms/InstCombine/vscale_load.ll

27

This is potentially undefined behaviour (the load can read beyond the alloca). A better option is for both tests to replace the alloca with a pointer parameter. That way the bounds of the storage are unknown and it becomes only about the store and load.

[peterwaller-arm]

• Update tests per review comments, use ptr param instead of alloca.

llvm/lib/Analysis/ConstantFolding.cpp

bool llvm::IsConstantOffsetFromGlobal(Constant *C, GlobalValue *&GV,
   Offset = TmpOffset;
   return true;
 }
 
 Constant *llvm::ConstantFoldLoadThroughBitcast(Constant *C, Type *DestTy,
                                          const DataLayout &DL) {
   do {
     Type *SrcTy = C->getType();
-    uint64_t DestSize = DL.getTypeSizeInBits(DestTy);
-    uint64_t SrcSize = DL.getTypeSizeInBits(SrcTy);
-    if (SrcSize < DestSize)
+    TypeSize DestSize = DL.getTypeSizeInBits(DestTy);
+    TypeSize SrcSize = DL.getTypeSizeInBits(SrcTy);
+    if (!TypeSize::isKnownGE(SrcSize, DestSize))
       return nullptr;
 
     // Catch the obvious splat cases (since all-zeros can coerce non-integral
     // pointers legally).
     if (C->isNullValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy())
       return Constant::getNullValue(DestTy);
     if (C->isAllOnesValue() &&
         (DestTy->isIntegerTy() || DestTy->isFloatingPointTy() ||

llvm/test/Transforms/InstCombine/vscale_load.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S -verify | FileCheck %s
+
+define <2 x i8> @constprop_load_bitcast() {
+; CHECK-LABEL: @constprop_load_bitcast(
+; CHECK-NEXT:    ret <2 x i8> zeroinitializer
+;
+  %stack_var = alloca <vscale x 16 x i1>, align 16
+  store <vscale x 16 x i1> zeroinitializer, <vscale x 16 x i1>* %stack_var, align 16
+  %cast_to_fixed = bitcast <vscale x 16 x i1>* %stack_var to <2 x i8>*
+  %a = load <2 x i8>, <2 x i8>* %cast_to_fixed, align 16
+  ret <2 x i8> %a
+}
+
+; vscale-sized vec not guaranteed to fill destination.
+define <8 x i8> @constprop_load_bitcast_neg() {
+; CHECK-LABEL: @constprop_load_bitcast_neg(
+; CHECK-NEXT:    [[STACK_VAR:%.*]] = alloca <vscale x 16 x i1>, align 16
+; CHECK-NEXT:    store <vscale x 16 x i1> zeroinitializer, <vscale x 16 x i1>* [[STACK_VAR]], align 16
+; CHECK-NEXT:    [[CAST_TO_FIXED:%.*]] = bitcast <vscale x 16 x i1>* [[STACK_VAR]] to <8 x i8>*
+; CHECK-NEXT:    [[A:%.*]] = load <8 x i8>, <8 x i8>* [[CAST_TO_FIXED]], align 16
+; CHECK-NEXT:    ret <8 x i8> [[A]]
+;
+  %stack_var = alloca <vscale x 16 x i1>, align 16
+  store <vscale x 16 x i1> zeroinitializer, <vscale x 16 x i1>* %stack_var, align 16
+  %cast_to_fixed = bitcast <vscale x 16 x i1>* %stack_var to <8 x i8>*
+  %a = load <8 x i8>, <8 x i8>* %cast_to_fixed, align 16
+  ret <8 x i8> %a
+}