[InstCombine] Handle vector compares in foldGEPIcmp(), take 2

[JesperAntonsson]

Summary

This is a continuation of the fix for PR34627 "InstCombine assertion at vector gep/icmp folding". (I just realized bugpoint had fuzzed the original test for me, so I had fixed another trigger of the same assert in adjacent code in InstCombine.)

This patch avoids optimizing an icmp (to look only at the base pointers) when the resulting icmp would have a different type.

The patch adds a testcase and also cleans up and shrinks the pre-existing test for the adjacent assert trigger.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineCompares.cpp

911 ↗

(On Diff #166883)

I don't know it, but wouldn't it be possible to compare the types themselves directly?
How does icmp ensure that the types are compatible?

[JesperAntonsson]

lib/Transforms/InstCombine/InstCombineCompares.cpp

911 ↗

(On Diff #166883)

With this patch, I'm trying to ensure the new and old ICmps have the same types, so that we can safely do replaceAllUsesWith(). That the two operands to the new ICmp have the same types is ensured already.

I can compare the ICmp types directly, but then I first have to create the new ICmp. What I do now is look at one operand of the new ICmp to check if it will get the same type as the old ICmp. E.g. if the old ICmp is {4 x i1] and the new operand is [4 x i16] then the new ICmp will have the same type as the old.

Perhaps it'd be cleaner to create the ICmp and then delete it and move on if it doesn't have the same type? I'll prepare and upload a patch with that for comparison.

[JesperAntonsson]

This patch checks icmp types directly. This might be simpler than checking the operand type.

[spatel]

lib/Transforms/InstCombine/InstCombineCompares.cpp

911 ↗

(On Diff #166883)

I think the API you're looking for is:
CmpInst::makeCmpResultType()

Creating and deleting an object is likely much more expensive than doing a type check.

[JesperAntonsson]

Thanks, Sanjay. Using makeCmpResultType() in this patch for a smaller and nicer diff.

[lebedev.ri]

Thanks, that is indeed what i expected to see.
One nit, but i'm really unsure of it.

lib/Transforms/InstCombine/InstCombineCompares.cpp

912 ↗

(On Diff #167714)

I'm not sure this helper variable is/isn't any better than without it?

[JesperAntonsson]

Thanks.

lib/Transforms/InstCombine/InstCombineCompares.cpp

912 ↗

(On Diff #167714)

I tried without and really felt the expression became too complex and unwieldy.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp

if (PtrBase != GEPRHS->getOperand(0)) {
       if (IndicesTheSame)
         for (unsigned i = 1, e = GEPLHS->getNumOperands(); i != e; ++i)
           if (GEPLHS->getOperand(i) != GEPRHS->getOperand(i)) {
             IndicesTheSame = false;
             break;
           }
 
       // If all indices are the same, just compare the base pointers.
-      if (IndicesTheSame)
+      Type *BaseType = GEPLHS->getOperand(0)->getType();
+      if (IndicesTheSame && CmpInst::makeCmpResultType(BaseType) == I.getType())
         return new ICmpInst(Cond, GEPLHS->getOperand(0), GEPRHS->getOperand(0));
 
       // If we're comparing GEPs with two base pointers that only differ in type
       // and both GEPs have only constant indices or just one use, then fold
       // the compare with the adjusted indices.
       if (GEPLHS->isInBounds() && GEPRHS->isInBounds() &&
           (GEPLHS->hasAllConstantIndices() || GEPLHS->hasOneUse()) &&
           (GEPRHS->hasAllConstantIndices() || GEPRHS->hasOneUse()) &&

llvm/trunk/test/Transforms/InstCombine/pr38984.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 target datalayout = "p:16:16"
 
-@offsets = external dso_local global [4 x i16], align 1
+@a = external global [21 x i16], align 1
+@offsets = external global [4 x i16], align 1
 
-define void @PR38984() {
-; CHECK-LABEL: @PR38984(
+; The "same gep" optimization should work with vector icmp.
+define <4 x i1> @PR38984_1() {
+; CHECK-LABEL: @PR38984_1(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    ret <4 x i1> <i1 true, i1 true, i1 true, i1 true>
 ;
 entry:
   %0 = load i16, i16* getelementptr ([4 x i16], [4 x i16]* @offsets, i16 0, i16 undef), align 1
   %1 = insertelement <4 x i16> undef, i16 %0, i32 3
-  %2 = sub <4 x i16> zeroinitializer, %1
-  %3 = sext <4 x i16> %2 to <4 x i32>
-  %4 = getelementptr inbounds i64, i64* null, <4 x i32> %3
-  %5 = ptrtoint <4 x i64*> %4 to <4 x i32>
-  %6 = getelementptr inbounds i64, i64* null, <4 x i16> %2
-  %7 = ptrtoint <4 x i64*> %6 to <4 x i32>
-  %8 = icmp eq <4 x i32> %5, %7
-  %9 = select <4 x i1> %8, <4 x i16> zeroinitializer, <4 x i16> <i16 1, i16 1, i16 1, i16 1>
-  %10 = sext <4 x i16> %9 to <4 x i32>
-  ret void
+  %2 = getelementptr i32, i32* null, <4 x i16> %1
+  %3 = getelementptr i32, i32* null, <4 x i16> %1
+  %4 = icmp eq <4 x i32*> %2, %3
+  ret <4 x i1> %4
+}
+
+; The "compare base pointers" optimization should not kick in for vector icmp.
+define <4 x i1> @PR38984_2() {
+; CHECK-LABEL: @PR38984_2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i16, i16* getelementptr ([4 x i16], [4 x i16]* @offsets, i16 0, i16 undef), align 2
+; CHECK-NEXT:    [[TMP1:%.*]] = insertelement <4 x i16> undef, i16 [[TMP0]], i32 3
+; CHECK-NEXT:    [[TMP2:%.*]] = getelementptr i16, i16* getelementptr inbounds ([21 x i16], [21 x i16]* @a, i16 1, i16 0), <4 x i16> [[TMP1]]
+; CHECK-NEXT:    [[TMP3:%.*]] = getelementptr i16, i16* null, <4 x i16> [[TMP1]]
+; CHECK-NEXT:    [[TMP4:%.*]] = icmp eq <4 x i16*> [[TMP2]], [[TMP3]]
+; CHECK-NEXT:    ret <4 x i1> [[TMP4]]
+;
+entry:
+  %0 = load i16, i16* getelementptr ([4 x i16], [4 x i16]* @offsets, i16 0, i16 undef)
+  %1 = insertelement <4 x i16> undef, i16 %0, i32 3
+  %2 = getelementptr i16, i16* getelementptr ([21 x i16], [21 x i16]* @a, i64 1, i32 0), <4 x i16> %1
+  %3 = getelementptr i16, i16* null, <4 x i16> %1
+  %4 = icmp eq <4 x i16*> %2, %3
+  ret <4 x i1> %4
 }