[InstSimplify] Constant fold icmp of gep

[nikic]

Summary

InstSimplify can fold icmps of gep where the base pointers are the same and the offsets are constant. It does so by constructing a constant expression icmp and assumes that it gets folded -- but this doesn't actually happen, because GEP expressions can usually only be folded by the target-dependent constant folding layer. As such, we need to explicitly invoke it here.

PS: Does someone know why we constant folding is separated in this way? I understand that we don't want to have ConstantExpr depend on TLI, but why can't it depend on DL?

[efriedma]

PS: Does someone know why we constant folding is separated in this way? I understand that we don't want to have ConstantExpr depend on TLI, but why can't it depend on DL?

This is sort of historical; deep in LLVM's history, we allowed generating/optimizing modules without a datalayout. This turned out to be mostly useless in practice, so we abandoned it many years ago. The two separate constant folders is one of the leftover traces of that.

It would be non-trivial to change at this point: in ConstantExpr::getICmp, there currently isn't any way to get a DataLayout.

lib/Analysis/InstructionSimplify.cpp

3487 ↗

(On Diff #247449)

If we need to repeat this pattern in multiple places, probably makes sense to factor this out into a helper.

[nikic]

In D75407#1901697, @efriedma wrote:

PS: Does someone know why we constant folding is separated in this way? I understand that we don't want to have ConstantExpr depend on TLI, but why can't it depend on DL?

This is sort of historical; deep in LLVM's history, we allowed generating/optimizing modules without a datalayout. This turned out to be mostly useless in practice, so we abandoned it many years ago. The two separate constant folders is one of the leftover traces of that.

It would be non-trivial to change at this point: in ConstantExpr::getICmp, there currently isn't any way to get a DataLayout.

Thanks for the historical context, that makes sense! Threading DL through 1.4k uses of ConstantExpr::get* indeed doesn't seem realistic at this point.

lib/Analysis/InstructionSimplify.cpp

3487 ↗

(On Diff #247449)

I was actually wondering whether we can make ConstantFoldConstant() simply always return non-null (either the original constant or a new one).

I initially thought that ConstantFoldConstant() follows the usual pattern where it returns non-null if and only if it actually folded something. However, this isn't actually the case: It returns null for non constexpr/constvector, but otherwise returns the original constant even if it didn't fold anything.

This seems like a not particularly useful in-between state and we could save this boilerplate (which is indeed repeated many times in the codebase) if we made ConstantFoldConstant() always return a constant.

[nikic]

Rebase and simplify.

[efriedma]

LGTM

llvm/lib/Analysis/InstructionSimplify.cpp

if (GEPOperator *GRHS = dyn_cast<GEPOperator>(RHS)) {
         Constant *Null = Constant::getNullValue(GLHS->getPointerOperandType());
         SmallVector<Value *, 4> IndicesLHS(GLHS->idx_begin(), GLHS->idx_end());
         Constant *NewLHS = ConstantExpr::getGetElementPtr(
             GLHS->getSourceElementType(), Null, IndicesLHS);
 
         SmallVector<Value *, 4> IndicesRHS(GRHS->idx_begin(), GRHS->idx_end());
         Constant *NewRHS = ConstantExpr::getGetElementPtr(
             GLHS->getSourceElementType(), Null, IndicesRHS);
-        return ConstantExpr::getICmp(Pred, NewLHS, NewRHS);
+        Constant *NewICmp = ConstantExpr::getICmp(Pred, NewLHS, NewRHS);
+        return ConstantFoldConstant(NewICmp, Q.DL);
       }
     }
   }
 
   // If the comparison is with the result of a select instruction, check whether
   // comparing with either branch of the select always yields the same value.
   if (isa<SelectInst>(LHS) || isa<SelectInst>(RHS))
     if (Value *V = ThreadCmpOverSelect(Pred, LHS, RHS, Q, MaxRecurse))

llvm/test/Transforms/InstSimplify/compare.ll

;
   %gep2 = getelementptr inbounds [4 x i8], [4 x i8]* %bc, i32 0, i32 1
   %pti2 = ptrtoint i8* %gep2 to i32
   %cmp = icmp ugt i32 %pti1, %pti2
   ret i1 %cmp
 }
 
 define i1 @gep_same_base_constant_indices(i8* %a) {
 ; CHECK-LABEL: @gep_same_base_constant_indices(
-; CHECK-NEXT:    ret i1 icmp slt (i8* getelementptr (i8, i8* null, i64 1), i8* getelementptr (i8, i8* null, i64 10))
+; CHECK-NEXT:    ret i1 true
 ;
   %arrayidx1 = getelementptr inbounds i8, i8* %a, i64 1
   %arrayidx2 = getelementptr inbounds i8, i8* %a, i64 10
   %cmp = icmp slt i8* %arrayidx1, %arrayidx2
   ret i1 %cmp
 }
 
 define i1 @zext(i32 %x) {