[ConstantFolding] Make sure constants are fully folded

[nikic]

Summary

ConstantFolding can currently return partially folded constants if certain folds create new GEP expressions. These GEPs may be created with incorrect index types by the DL-independent layer, in which case ConstantFolding will change them to be DL-conforming.

I'm fixing this by explicitly re-running ConstantFolding in the two places I have found where this can occur (they create zero-index GEPs from bitcasts). I have to say that this is fairly ugly (especially as some care has to be taken to avoid infinite recursion), but I'm not sure how else to handle this.

[nikic]

I encountered some more cases where we can end up with not fully folded constants, in particular folding of load instructions will apparently not fold values loaded from aggregates. Looking through the code, the same probably happens when phis are folded. As such, it may not make sense to try and address this in individual places, and we should instead automatically perform another constant fold if constant folding succeeded. Alternatively we could say that constant folding doesn't have to be complete in the first place (which was my assumption) and the consumer should rerun it multiple times instead (which seems pretty odd to me).

[efriedma]

Making constant folding recurse after it transforms something probably makes sense.

Practically, I'm not sure how much it matters, though. "constprop" is not part of the normal pass pipeline; instcombine will iterate until it reaches a steady state.

[nikic]

In D75408#1901690, @efriedma wrote:

Making constant folding recurse after it transforms something probably makes sense.

Practically, I'm not sure how much it matters, though. "constprop" is not part of the normal pass pipeline; instcombine will iterate until it reaches a steady state.

Avoiding those extra fix-point iterations is really the goal here :) Constant folding in particular can fall into a small crack though, where we don't actually end up at a fixpoint after InstCombine finishes. An example: https://llvm.godbolt.org/z/V2bJYP. The reason is that constant operand folding is only performed during initial worklist population, presumably on the assumption that it does not require fixpoint iteration.

[nikic]

Rerun constant folding in ConstantFoldInstruction and ConstantFoldConstant to make sure the result is fully folded.

I'm wondering whether I should push this further back into APIs like ConstantFoldInstOperands, which are also public. Might be a bit ugly implementation-wise, and we'll lose FoldedOps along the way.

[nikic]

I think the motivation for doing this has largely gone away by now.

lib/Analysis/ConstantFolding.cpp

if (isa<ConstantVector>(OldC) || isa<ConstantExpr>(OldC)) {
       } else {
         NewC = It->second;
       }
     }
     Ops.push_back(NewC);
   }
 
   if (auto *CE = dyn_cast<ConstantExpr>(C)) {
+    Constant *Res;
     if (CE->isCompare())
-      return ConstantFoldCompareInstOperands(CE->getPredicate(), Ops[0], Ops[1],
-                                             DL, TLI);
+      Res = ConstantFoldCompareInstOperands(CE->getPredicate(), Ops[0], Ops[1],
+    else
+      Res = ConstantFoldInstOperandsImpl(CE, CE->getOpcode(), Ops, DL, TLI);
 
-    return ConstantFoldInstOperandsImpl(CE, CE->getOpcode(), Ops, DL, TLI);
+    // Ensure the constant is fully folded.
+    if (Res != C)
+      Res = ConstantFoldConstantImpl(Res, DL, TLI, FoldedOps);
+    return Res;
   }
 
   assert(isa<ConstantVector>(C));
   return ConstantVector::get(Ops);
 }
 
 } // end anonymous namespace
 
Constant *llvm::ConstantFoldInstruction(Instruction *I, const DataLayout &DL,
   SmallVector<Constant *, 8> Ops;
   for (const Use &OpU : I->operands()) {
     auto *Op = cast<Constant>(&OpU);
     // Fold the Instruction's operands.
     Op = ConstantFoldConstantImpl(Op, DL, TLI, FoldedOps);
     Ops.push_back(Op);
   }
 
+  Constant *Res;
   if (const auto *CI = dyn_cast<CmpInst>(I))
-    return ConstantFoldCompareInstOperands(CI->getPredicate(), Ops[0], Ops[1],
-                                           DL, TLI);
-
-  if (const auto *LI = dyn_cast<LoadInst>(I))
-    return ConstantFoldLoadInst(LI, DL);
-
-  if (auto *IVI = dyn_cast<InsertValueInst>(I)) {
-    return ConstantExpr::getInsertValue(
-                                cast<Constant>(IVI->getAggregateOperand()),
-                                cast<Constant>(IVI->getInsertedValueOperand()),
-                                IVI->getIndices());
-  }
-
-  if (auto *EVI = dyn_cast<ExtractValueInst>(I)) {
-    return ConstantExpr::getExtractValue(
-                                    cast<Constant>(EVI->getAggregateOperand()),
-                                    EVI->getIndices());
-  }
+    Res = ConstantFoldCompareInstOperands(CI->getPredicate(), Ops[0], Ops[1],
+  else if (const auto *LI = dyn_cast<LoadInst>(I))
+    Res = ConstantFoldLoadInst(LI, DL);
+  else if (auto *IVI = dyn_cast<InsertValueInst>(I))
+    Res = ConstantExpr::getInsertValue(
+  else if (auto *EVI = dyn_cast<ExtractValueInst>(I))
+    Res = ConstantExpr::getExtractValue(
+        cast<Constant>(EVI->getAggregateOperand()), EVI->getIndices());
+  else
+    Res = ConstantFoldInstOperands(I, Ops, DL, TLI);
 
-  return ConstantFoldInstOperands(I, Ops, DL, TLI);
+  // Ensure the constant is fully folded.
+  if (Res)
+    return ConstantFoldConstantImpl(Res, DL, TLI, FoldedOps);
+  return nullptr;
 }
 
 Constant *llvm::ConstantFoldConstant(const Constant *C, const DataLayout &DL,
                                      const TargetLibraryInfo *TLI) {
   SmallDenseMap<Constant *, Constant *> FoldedOps;
   return ConstantFoldConstantImpl(C, DL, TLI, FoldedOps);
 }
 

test/Analysis/ConstantFolding/gep-zeroinit-vector.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -constprop -S -o - | FileCheck %s
 
 ; Testcase that verify that we don't get a faulty bitcast that cast between
 ; different sizes.
 
 %rec8 = type { i16 }
 @a = global [1 x %rec8] zeroinitializer
 
 define <2 x i16*> @test_gep() {
 ; CHECK-LABEL: @test_gep(
-; CHECK-NEXT:    ret <2 x i16*> <i16* getelementptr inbounds ([1 x %rec8], [1 x %rec8]* @a, i32 0, i32 0, i32 0), i16* getelementptr inbounds ([1 x %rec8], [1 x %rec8]* @a, i32 0, i32 0, i32 0)>
+; CHECK-NEXT:    ret <2 x i16*> <i16* getelementptr inbounds ([1 x %rec8], [1 x %rec8]* @a, i64 0, i64 0, i32 0), i16* getelementptr inbounds ([1 x %rec8], [1 x %rec8]* @a, i64 0, i64 0, i32 0)>
 ;
   %A = getelementptr [1 x %rec8], [1 x %rec8]* @a, <2 x i16> zeroinitializer, <2 x i64> zeroinitializer
   %B = bitcast <2 x %rec8*> %A to <2 x i16*>
   ret <2 x i16*> %B
 }
 
 ; Testcase that verify the cast-of-cast when the outer/second cast is to a
 ; vector type.

test/Transforms/ConstProp/bitcast.ll

;
   ret i1 %cmp
 }
 
 %T = type { i8 }
 @G = external global %T
 
 define i8* @bitcast_to_gep() {
 ; CHECK-LABEL: @bitcast_to_gep(
-; CHECK-NEXT:    ret i8* getelementptr inbounds (%T, %T* @G, i32 0, i32 0)
+; CHECK-NEXT:    ret i8* getelementptr inbounds (%T, %T* @G, i64 0, i32 0)
 ;
   %p = bitcast %T* @G to i8*
   ret i8* %p
 }
 
 define i8 addrspace(1)* @addrspacecast_to_gep() {
 ; CHECK-LABEL: @addrspacecast_to_gep(
-; CHECK-NEXT:    ret i8 addrspace(1)* addrspacecast (i8* getelementptr inbounds (%T, %T* @G, i32 0, i32 0) to i8 addrspace(1)*)
+; CHECK-NEXT:    ret i8 addrspace(1)* addrspacecast (i8* getelementptr inbounds (%T, %T* @G, i64 0, i32 0) to i8 addrspace(1)*)
 ;
   %p = addrspacecast %T* @G to i8 addrspace(1)*
   ret i8 addrspace(1)* %p
 }