[GVN] Fix handling of sub-byte types in big-endian mode

[uweigand]

Summary

When GVN wants to re-interpret an already available value in a smaller
type, it needs to right-shift the value on big-endian systems to ensure
the correct bytes are accessed. The shift value is the difference of
the sizes of the two types.

This is correct as long as both types occupy multiples of full bytes.
However, when one of them is a sub-byte type like i1, this no longer
holds true: we still need to shift, but only to access the correct
*byte*. Accessing bits within the byte requires no shift in either
endianness; e.g. an i1 resides in the least-significant bit of its
containing byte on both big- and little-endian systems.

Therefore, the appropriate shift value to be used is the difference of
the *storage* sizes of the two types. This is already handled correctly
in one place where such a shift takes place (GetStoreValueForLoad), but
is incorrect in two other places: GetLoadValueForLoad and
CoerceAvailableValueToLoadType.

This patch changes both places to use the storage size as well.

[chandlerc]

Very nice fix. Minor complaints below about variable naming, feel free to address and commit as you see fit.

lib/Transforms/Scalar/GVN.cpp

778 ↗

(On Diff #52251)

You know, "StoreSize" and "LoadSize" are really bad variable names since they aren't actually the type store size... Which makes much more sense out of how this bug got in there in the first place.

Maybe rename them (in a separate patch) to something more clearly to do with value sizes rather than store and load sizes?

1061 ↗

(On Diff #52251)

Here, we see the reverse confusion IMO: SrcValSize would be better named SrcValStoreSize.

[uweigand]

In D18662#394154, @chandlerc wrote:

Very nice fix. Minor complaints below about variable naming, feel free to address and commit as you see fit.

Thanks for the review! I've now checked in the original patch, as well as a separate commit to rename those variables.

llvm/trunk/lib/Transforms/Scalar/GVN.cpp

static Value *CoerceAvailableValueToLoadType(Value *StoredVal, Type *LoadedTy,
   if (!StoredValTy->isIntegerTy()) {
     StoredValTy = IntegerType::get(StoredValTy->getContext(), StoreSize);
     StoredVal = IRB.CreateBitCast(StoredVal, StoredValTy);
   }
 
   // If this is a big-endian system, we need to shift the value down to the low
   // bits so that a truncate will work.
   if (DL.isBigEndian()) {
-    StoredVal = IRB.CreateLShr(StoredVal, StoreSize - LoadSize, "tmp");
+    uint64_t ShiftAmt = DL.getTypeStoreSizeInBits(StoredValTy) -
+                        DL.getTypeStoreSizeInBits(LoadedTy);
+    StoredVal = IRB.CreateLShr(StoredVal, ShiftAmt, "tmp");
   }
 
   // Truncate the integer to the right size now.
   Type *NewIntTy = IntegerType::get(StoredValTy->getContext(), LoadSize);
   StoredVal  = IRB.CreateTrunc(StoredVal, NewIntTy, "trunc");
 
   if (LoadedTy == NewIntTy)
     return StoredVal;
if (Offset+LoadSize > SrcValSize) {
 
     DEBUG(dbgs() << "GVN WIDENED LOAD: " << *SrcVal << "\n");
     DEBUG(dbgs() << "TO: " << *NewLoad << "\n");
 
     // Replace uses of the original load with the wider load.  On a big endian
     // system, we need to shift down to get the relevant bits.
     Value *RV = NewLoad;
     if (DL.isBigEndian())
-      RV = Builder.CreateLShr(RV,
-                    NewLoadSize*8-SrcVal->getType()->getPrimitiveSizeInBits());
+      RV = Builder.CreateLShr(RV, (NewLoadSize - SrcValSize) * 8);
     RV = Builder.CreateTrunc(RV, SrcVal->getType());
     SrcVal->replaceAllUsesWith(RV);
 
     // We would like to use gvn.markInstructionForDeletion here, but we can't
     // because the load is already memoized into the leader map table that GVN
     // tracks.  It is potentially possible to remove the load from the table,
     // but then there all of the operations based on it would need to be
     // rehashed.  Just leave the dead load around.

llvm/trunk/test/Transforms/GVN/big-endian.ll

+; RUN: opt -gvn -S < %s | FileCheck %s
+
+target datalayout = "E-m:e-i64:64-n32:64"                                                                                         
+target triple = "powerpc64-unknown-linux-gnu"                                                                                     
+
+;; Make sure we use correct bit shift based on storage size for
+;; loads reusing a load value.
+define i64 @test1({ i1, i8 }* %predA, { i1, i8 }* %predB) {
+; CHECK-LABEL: @test1
+; CHECK: [[V1:%.*]] = load i16, i16* %{{.*}}
+; CHECK: [[V2:%.*]] = lshr i16 [[V1]], 8
+; CHECK: trunc i16 [[V2]] to i1
+
+  %valueLoadA.fca.0.gep = getelementptr inbounds { i1, i8 }, { i1, i8 }* %predA, i64 0, i32 0
+  %valueLoadA.fca.0.load = load i1, i1* %valueLoadA.fca.0.gep, align 8
+  %valueLoadB.fca.0.gep = getelementptr inbounds { i1, i8 }, { i1, i8 }* %predB, i64 0, i32 0
+  %valueLoadB.fca.0.load = load i1, i1* %valueLoadB.fca.0.gep, align 8
+  %isTrue = and i1 %valueLoadA.fca.0.load, %valueLoadB.fca.0.load
+  %valueLoadA.fca.1.gep = getelementptr inbounds { i1, i8 }, { i1, i8 }* %predA, i64 0, i32 1
+  %valueLoadA.fca.1.load = load i8, i8* %valueLoadA.fca.1.gep, align 1
+  %isNotNullA = icmp ne i8 %valueLoadA.fca.1.load, 0
+  %valueLoadB.fca.1.gep = getelementptr inbounds { i1, i8 }, { i1, i8 }* %predB, i64 0, i32 1
+  %valueLoadB.fca.1.load = load i8, i8* %valueLoadB.fca.1.gep, align 1
+  %isNotNullB = icmp ne i8 %valueLoadB.fca.1.load, 0
+  %isNotNull = and i1 %isNotNullA, %isNotNullB
+  %isTrueAndNotNull = and i1 %isTrue, %isNotNull
+  %ret = zext i1 %isTrueAndNotNull to i64
+  ret i64 %ret
+}
+
+;; And likewise for loads reusing a store value.
+define i1 @test2(i8 %V, i8* %P) {
+; CHECK-LABEL: @test2
+; CHECK-NOT: lshr
+  store i8 %V, i8* %P
+  %P2 = bitcast i8* %P to i1*
+  %A = load i1, i1* %P2
+  ret i1 %A
+}
+