[GVN] Avoid casting a vector of size less than 8 bits to i8

[ormris]

Summary

A reprise of D25849.

This crash was found through fuzzing some time ago and was documented in PR28879.

No check for load size has been added due to the following tests:

• Transforms/GVN/invariant.group.ll
• Transforms/GVN/pr10820.ll

These tests expect load sizes that are not a multiple of eight.

Thanks to @davide for the original patch.

[efriedma]

It should be possible to implement this sort of coercion with an appropriate cast, rather than just bailing out, I think. But I won't block the patch on that.

Please add a testcase for the load-load case, in addition to the load-store case.

[RKSimon]

test/Transforms/GVN/pr28879.ll

1 ↗

(On Diff #151958)

FileCheck? I realise its a crash test but some sanity checks could be helpful

[ormris]

@efriedma I'm not quite sure what that would look like here. Are you looking for something like the following?

%a = alloca <7 x i1>, align 2
store <7 x i1> undef, <7 x i1>* %a, align 2
%0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
%val0 = load i1, i1* %0, align 2
%val1 = load i1, i1* %0, align 2
br i1 %val0, label %cond.true, label %cond.false

test/Transforms/GVN/pr28879.ll

1 ↗

(On Diff #151958)

Agreed. I'll take a look at it.

[efriedma]

For the test, I mean something like the following:

define <7 x i1> @f(<7 x i1>* %a) {
entry:
  %vec = load <7 x i1>, <7 x i1>* %a
  %0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
  %val = load i1, i1* %0, align 2
  br i1 %val, label %cond.true, label %cond.false

cond.true:
  ret <7 x i1> %vec

cond.false:
  ret <7 x i1> <i1 false, i1 false, i1 false, i1 false, i1 false, i1 false, i1 false>
}

[ormris]

Add load-load case and sanity checks

[efriedma]

lib/Transforms/Utils/VNCoercion.cpp

25 ↗

(On Diff #152364)

Please add a brief comment here explaining why we bail out.

test/Transforms/GVN/load-load-odd-vector.ll

3 ↗

(On Diff #152364)

Please put this test into the same file as the other test.

[ormris]

Merge two tests. Add comment.

[efriedma]

LGTM

[ormris]

Thanks for the review @efriedma and @RKSimon! I'll go ahead and commit this.

llvm/trunk/lib/Transforms/Utils/VNCoercion.cpp

 bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
                                      const DataLayout &DL) {
   // If the loaded or stored value is an first class array or struct, don't try
   // to transform them.  We need to be able to bitcast to integer.
   if (LoadTy->isStructTy() || LoadTy->isArrayTy() ||
       StoredVal->getType()->isStructTy() || StoredVal->getType()->isArrayTy())
     return false;
 
+  uint64_t StoreSize = DL.getTypeSizeInBits(StoredVal->getType());
+
+  // The store size must be byte-aligned to support future type casts.
+  if (llvm::alignTo(StoreSize, 8) != StoreSize)
+    return false;
+
   // The store has to be at least as big as the load.
-  if (DL.getTypeSizeInBits(StoredVal->getType()) < DL.getTypeSizeInBits(LoadTy))
+  if (StoreSize < DL.getTypeSizeInBits(LoadTy))
     return false;
 
   // Don't coerce non-integral pointers to integers or vice versa.
   if (DL.isNonIntegralPointerType(StoredVal->getType()) !=
       DL.isNonIntegralPointerType(LoadTy))
     return false;
 
   return true;

llvm/trunk/test/Transforms/GVN/pr28879.ll

+; RUN: opt -gvn <%s -S -o - | FileCheck %s
+
+define void @f() {
+entry:
+  %a = alloca <7 x i1>, align 2
+  store <7 x i1> undef, <7 x i1>* %a, align 2
+; CHECK: store <7 x i1> undef, <7 x i1>*
+  %0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
+  %val = load i1, i1* %0, align 2
+; CHECK: load i1, i1* 
+  br i1 %val, label %cond.true, label %cond.false
+
+cond.true:
+  ret void
+
+cond.false:
+  ret void
+}
+
+define <7 x i1> @g(<7 x i1>* %a) {
+entry:
+  %vec = load <7 x i1>, <7 x i1>* %a
+; CHECK: load <7 x i1>, <7 x i1>*
+  %0 = getelementptr inbounds <7 x i1>, <7 x i1>* %a, i64 0, i64 0
+  %val = load i1, i1* %0, align 2
+; CHECK: load i1, i1*
+  br i1 %val, label %cond.true, label %cond.false
+
+cond.true:
+  ret <7 x i1> %vec
+
+cond.false:
+  ret <7 x i1> <i1 false, i1 false, i1 false, i1 false, i1 false, i1 false, i1 false>
+}