[ConstantFolding] Unify handling of load from uniform value

[nikic]

Summary

There are a number of places that specially handle loads from a uniform value where all the bits are the same (zero, one, undef, poison), because we a) don't care about the load offset in that case b) it bypasses casts that might not be legal generally but do work with uniform values.

We had multiple implementations of this, with a different set of supported values each time, as well as incomplete type checks in some cases. In particular, this fixes the assertion reported in https://reviews.llvm.org/D114889#3198921, as well as a similar assertion that could be triggered via constant folding.

[ayrivera]

llvm/lib/Analysis/ConstantFolding.cpp

718

Is there a chance that Ty is not X86_MMX neither X86_AMX and C is NullValue(), but getNullValue doesn't support the type? For example, assume that Ty is a Function type, then in that case getNullValue will default into the llvm_unreachable.

722

If Ty is integer, or FP, or vector, can it be X86_AMX, X86_MMX or PtrOrPtrVector? Of not, then I think the extra check can be removed.

[nikic]

Simplify condition.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

718

I believe other problematic types cannot appear as a load or bitcast type. You can't load a function type (only a pointer to a function type).

722

Yeah, this condition was too complicated. We only need to check for int, fp or vector of them.

[ayrivera]

LGTM! Thanks for the quick fix.

llvm/lib/Analysis/ConstantFolding.cpp

718

I see, thanks for the explanation.

[paulwalker-arm]

This patch looks to be breaking the clang-aarch64-sve-vla-2stage buildbot (and probably also clang-aarch64-sve-vls-2stage). I've checked this using ./bin/clang -DNDEBUG -O3 -w -Werror=date-time -w pr19687.c && ./a.out && echo "success", which works before this patch but triggers an abort after. This happen for both AArch64 and X86.

See https://lab.llvm.org/buildbot/#/builders/198/builds/511 and https://lab.llvm.org/buildbot/#/builders/176/builds/1231.

[nikic]

@paulwalker-arm Thanks for the report, I've reverted the commit for now. I've put up D115994 to either fix the test or be told that clang generates incorrect initialization code, I'm not completely sure which it is.

[nikic]

In the meantime, I've applied https://github.com/llvm/llvm-project/commit/2926d6d335aca7e3f57ac45e6b25b1716e053fb3 as a targeted fix for the original assertion failures.

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

722

Sorry for the late comment, but is this legal to do if the src and dest types are different sizes? E.g.:

%xxx_cast = bitcast i8* %xxx to i1*
store i1 true, i1* %xxx_cast
%yyy = load i8, i8* %xxx

In this case, we'll be turning an i1 -1 into an i8 -1, which changes bits.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

722

This code assumes that the loaded type is either smaller or that loading a larger type fills in the remaining bits with poison, so we can use any value for them. The caller is responsible for doing a type size check if necessary.

However, I don't believe that non-byte-sized types were really considered either here or in other parts of the constant load folding code. In that case the type store sizes are the same, but the type sizes differ.

Now, as to whether this behavior is actually incorrect, LangRef has the following to say on non-byte-sized memory accesses:

When writing a value of a type like i20 with a size that is not an integral number of bytes, it is unspecified what happens to the extra bits that do not belong to the type, but they will typically be overwritten.

When loading a value of a type like i20 with a size that is not an integral number of bytes, the result is undefined if the value was not originally written using a store of the same type.

Based on a strict reading, I believe the store of i1 and load of i8 would result in the remaining bits having an unspecified, but generally non-poison value. The reverse would be UB (which really doesn't make sense to me -- it would be great if we could rework this to be more well-defined.)

So, yeah, I'd say this is a bug. I'll take a look.

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

722

Thanks, Nikita.

Looking at the LangRef language, I suspect that you're correct here:

Based on a strict reading, I believe the store of i1 and load of i8 would result in the remaining bits having an unspecified, but generally non-poison value.

Requiring the IR producer to maintain those unspecified bits is an acceptable answer. ;)

I wish LangRef took the responsibility of maintaining the unspecified i1/i4 bits off of the IR producer, since they're so common in predication, but I also understand the access instruction limitations as well. It's an unfortunate situation.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

722

I've landed a partial fix in https://github.com/llvm/llvm-project/commit/930a68765dff96927d706d258ef0c2ad9c7ec2ab, because this was checking the wrong type sizes.

I plan to also add handling for this in the constant folding code though, to also fix this variant of the problem: https://github.com/llvm/llvm-project/commit/659871cede9e3475c5de986ba4cace58e70f4801#diff-cc91356612b63dff2481358f87d5da7e98d7bbf8fc65c80e55d55c20b1dba462

clang/test/CodeGen/aapcs-align.cpp

 void f6(int, P8);
 void f6m(int, int, int, int, int, P8);
 void g6() {
   P8 s = {6, 7};
   f6(1, s);
   f6m(1, 2, 3, 4, 5, s);
 }
 // CHECK: define{{.*}} void @g6
-// CHECK: call void @f6(i32 1, [4 x i32] [i32 6, i32 7, i32 0, i32 0])
-// CHECK: call void @f6m(i32 1, i32 2, i32 3, i32 4, i32 5, [4 x i32] [i32 6, i32 7, i32 0, i32 0])
+// CHECK: call void @f6(i32 1, [4 x i32] [i32 6, i32 7, i32 0, i32 undef])
+// CHECK: call void @f6m(i32 1, i32 2, i32 3, i32 4, i32 5, [4 x i32] [i32 6, i32 7, i32 0, i32 undef])
 // CHECK: declare void @f6(i32, [4 x i32])
 // CHECK: declare void @f6m(i32, i32, i32, i32, i32, [4 x i32])
 }

llvm/include/llvm/Analysis/ConstantFolding.h

 Constant *ConstantFoldLoadFromConstPtr(Constant *C, Type *Ty, APInt Offset,
                                        const DataLayout &DL);
 
 /// Return the value that a load from C would produce if it is constant and
 /// determinable. If this is not determinable, return null.
 Constant *ConstantFoldLoadFromConstPtr(Constant *C, Type *Ty,
                                        const DataLayout &DL);
 
+/// If C is a uniform value where all bits are the same (either all zero, all
+/// ones, all undef or all poison), return the corresponding uniform value in
+/// the new type. If the value is not uniform or the result cannot be
+/// represented, return null.
+Constant *ConstantFoldLoadFromUniformValue(Constant *C, Type *Ty);
+
 /// ConstantFoldLoadThroughGEPConstantExpr - Given a constant and a
 /// getelementptr constantexpr, return the constant value being addressed by the
 /// constant expression, or null if something is funny and we can't decide.
 Constant *ConstantFoldLoadThroughGEPConstantExpr(Constant *C, ConstantExpr *CE,
                                                  Type *Ty,
                                                  const DataLayout &DL);
 
 /// canConstantFoldCallTo - Return true if its even possible to fold a call to

llvm/lib/Analysis/ConstantFolding.cpp

 /// Constant fold bitcast, symbolically evaluating it with DataLayout.
 /// This always returns a non-null constant, but it may be a
 /// ConstantExpr if unfoldable.
 Constant *FoldBitCast(Constant *C, Type *DestTy, const DataLayout &DL) {
   assert(CastInst::castIsValid(Instruction::BitCast, C, DestTy) &&
          "Invalid constantexpr bitcast!");
 
   // Catch the obvious splat cases.
-  if (C->isNullValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy())
-    return Constant::getNullValue(DestTy);
-  if (C->isAllOnesValue() && !DestTy->isX86_MMXTy() && !DestTy->isX86_AMXTy() &&
-      !DestTy->isPtrOrPtrVectorTy()) // Don't get ones for ptr types!
-    return Constant::getAllOnesValue(DestTy);
+  if (Constant *Res = ConstantFoldLoadFromUniformValue(C, DestTy))
+    return Res;
 
   if (auto *VTy = dyn_cast<VectorType>(C->getType())) {
     // Handle a vector->scalar integer/fp cast.
     if (isa<IntegerType>(DestTy) || DestTy->isFloatingPointTy()) {
       unsigned NumSrcElts = cast<FixedVectorType>(VTy)->getNumElements();
       Type *SrcEltTy = VTy->getElementType();
 
       // If the vector is a vector of floating point, convert it to vector of int
do {
 
     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() ||
-         DestTy->isVectorTy()) &&
-        !DestTy->isX86_AMXTy() && !DestTy->isX86_MMXTy() &&
-        !DestTy->isPtrOrPtrVectorTy())
-      // Get ones when the input is trivial, but
-      // only for supported types inside getAllOnesValue.
-      return Constant::getAllOnesValue(DestTy);
+    if (Constant *Res = ConstantFoldLoadFromUniformValue(C, DestTy))
+      return Res;
 
     // If the type sizes are the same and a cast is legal, just directly
     // cast the constant.
     // But be careful not to coerce non-integral pointers illegally.
     if (SrcSize == DestSize &&
         DL.isNonIntegralPointerType(SrcTy->getScalarType()) ==
             DL.isNonIntegralPointerType(DestTy->getScalarType())) {
       Instruction::CastOps Cast = Instruction::BitCast;
C = cast<Constant>(C->stripAndAccumulateConstantOffsets(
           DL, Offset, /* AllowNonInbounds */ true));
 
   if (auto *GV = dyn_cast<GlobalVariable>(C))
     if (GV->isConstant() && GV->hasDefinitiveInitializer())
       if (Constant *Result = ConstantFoldLoadFromConst(GV->getInitializer(), Ty,
                                                        Offset, DL))
         return Result;
 
-  // If this load comes from anywhere in a constant global, and if the global
-  // is all undef or zero, we know what it loads.
-  if (auto *GV = dyn_cast<GlobalVariable>(getUnderlyingObject(C))) {
-    if (GV->isConstant() && GV->hasDefinitiveInitializer()) {
-      if (GV->getInitializer()->isNullValue())
-        return Constant::getNullValue(Ty);
-      if (isa<UndefValue>(GV->getInitializer()))
-        return UndefValue::get(Ty);
-    }
-  }
+  // If this load comes from anywhere in a uniform constant global, the value
+  // is always the same, regardless of the loaded offset.
+  if (auto *GV = dyn_cast<GlobalVariable>(getUnderlyingObject(C)))
+    if (GV->isConstant() && GV->hasDefinitiveInitializer())
+      if (Constant *Res =
+              ConstantFoldLoadFromUniformValue(GV->getInitializer(), Ty))
+        return Res;
 
   return nullptr;
 }
 
 Constant *llvm::ConstantFoldLoadFromConstPtr(Constant *C, Type *Ty,
                                              const DataLayout &DL) {
   APInt Offset(DL.getIndexTypeSizeInBits(C->getType()), 0);
   return ConstantFoldLoadFromConstPtr(C, Ty, Offset, DL);
 }
 
+Constant *llvm::ConstantFoldLoadFromUniformValue(Constant *C, Type *Ty) {
+  if (isa<PoisonValue>(C))
+    return PoisonValue::get(Ty);
+  if (isa<UndefValue>(C))
+    return UndefValue::get(Ty);
+  if (C->isNullValue() && !Ty->isX86_MMXTy() && !Ty->isX86_AMXTy())
+    return Constant::getNullValue(Ty);
+  if (C->isAllOnesValue() &&
+      (Ty->isIntOrIntVectorTy() || Ty->isFPOrFPVectorTy()))
+    return Constant::getAllOnesValue(Ty);
+  return nullptr;
+}
+
 namespace {
 
 /// One of Op0/Op1 is a constant expression.
 /// Attempt to symbolically evaluate the result of a binary operator merging
 /// these together.  If target data info is available, it is provided as DL,
 /// otherwise DL is null.
 Constant *SymbolicallyEvaluateBinop(unsigned Opc, Constant *Op0, Constant *Op1,
                                     const DataLayout &DL) {

llvm/lib/Transforms/IPO/GlobalOpt.cpp

if (auto *BO = dyn_cast<BitCastOperator>(U))
       append_range(WorkList, BO->users());
     if (auto *ASC = dyn_cast<AddrSpaceCastOperator>(U))
       append_range(WorkList, ASC->users());
     else if (auto *GEP = dyn_cast<GEPOperator>(U))
       append_range(WorkList, GEP->users());
     else if (auto *LI = dyn_cast<LoadInst>(U)) {
       // A load from zeroinitializer is always zeroinitializer, regardless of
       // any applied offset.
-      if (Init->isNullValue()) {
-        LI->replaceAllUsesWith(Constant::getNullValue(LI->getType()));
+      if (Constant *Res =
+              ConstantFoldLoadFromUniformValue(Init, LI->getType())) {
+        LI->replaceAllUsesWith(Res);
         EraseFromParent(LI);
         continue;
       }
 
       Value *PtrOp = LI->getPointerOperand();
       APInt Offset(DL.getIndexTypeSizeInBits(PtrOp->getType()), 0);
       PtrOp = PtrOp->stripAndAccumulateConstantOffsets(
           DL, Offset, /* AllowNonInbounds */ true);

llvm/test/Transforms/GlobalOpt/x86_mmx_load.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -globalopt < %s | FileCheck %s
+
+@m64 = internal global <1 x i64> zeroinitializer
+
+define i32 @load_mmx() {
+; CHECK-LABEL: @load_mmx(
+; CHECK-NEXT:    ret i32 0
+;
+  %temp = load x86_mmx, x86_mmx* bitcast (<1 x i64>* @m64 to x86_mmx*)
+  ret i32 0
+}

llvm/test/Transforms/InstSimplify/ConstProp/loads.ll

 
 define { i64, i64 } @test_load_struct() {
 ; CHECK-LABEL: @test_load_struct(
 ; CHECK-NEXT:    ret { i64, i64 } { i64 123, i64 112312312 }
 ;
   %v = load { i64, i64 }, { i64, i64 }* @g3
   ret { i64, i64 } %v
 }
+
+@m64 = internal constant [2 x i64] zeroinitializer
+@idx = external global i32
+
+; This should not try to create an x86_mmx null value.
+define x86_mmx @load_mmx() {
+; CHECK-LABEL: @load_mmx(
+; CHECK-NEXT:    [[TEMP:%.*]] = load x86_mmx, x86_mmx* bitcast (i64* getelementptr ([2 x i64], [2 x i64]* @m64, i64 0, i64 ptrtoint (i32* @idx to i64)) to x86_mmx*), align 8
+; CHECK-NEXT:    ret x86_mmx [[TEMP]]
+;
+  %temp = load x86_mmx, x86_mmx* bitcast (i64* getelementptr ([2 x i64], [2 x i64]* @m64, i64 0, i64 ptrtoint (i32* @idx to i64)) to x86_mmx*)
+  ret x86_mmx %temp
+}