[ConstantFold] Turn off expression simplification for vector type

[Ka-Ka]

Summary

The getelementptr returns a vector of pointers, instead of a single
address, when one or more of its arguments is a vector. In such case it
is not possible to simplify the expression by inserting a bitcast of
operand(0) into the destination type, as it will create a bitcast
between different sizes.

The simplifications turned off in this patch was not intended for
vectorized types.

[Ka-Ka]

Ping!
Anyone up for reviewing this?

[lebedev.ri]

test/Transforms/LoopVectorize/X86/constant-fold.ll

1 ↗

(On Diff #144985)

1. Use utils/update_test_checks.py
2. Commit the patch with check-lines as of trunk.
3. Rebase the differential

[Ka-Ka]

Changed testcase according what was suggested by lebedev.ri

[Ka-Ka]

Ping?

[Ka-Ka]

Adding Matthew Simpson and Sanjay Patel as reviewers also as they have done work in ConstantFold recently.

[spatel]

This is a constant folding change, so the test should be visible with just "opt -constprop ..."

Ie, the test should be minimized and moved under test/Analysis/ConstantFolding.

I don't know this code. Is the bug only related to GEP? But this patch also affects cast-of-cast when the outer/second cast is to a vector type? Eg, you don't want to inhibit this:

define <4 x i16> @test_mmx_const() {
  %A = bitcast <2 x i32> zeroinitializer to x86_mmx
  %B = bitcast x86_mmx %A to <4 x i16>
  ret <4 x i16> %B
}

$ opt -constprop foo.ll -S
define <4 x i16> @test_mmx_const() {
  ret <4 x i16> zeroinitializer
}

(so there should be a negative test for that example in this patch)

[Ka-Ka]

In D46379#1118168, @spatel wrote:

This is a constant folding change, so the test should be visible with just "opt -constprop ..."

Ie, the test should be minimized and moved under test/Analysis/ConstantFolding.

I agree, it is much better with a smaller testcase. It turned out to be much easier than I initially thought to create a testcase with "opt -constprop ..."

I don't know this code. Is the bug only related to GEP?
But this patch also affects cast-of-cast when the outer/second cast is to a vector type? Eg, you don't want to inhibit this:

define <4 x i16> @test_mmx_const() {
  %A = bitcast <2 x i32> zeroinitializer to x86_mmx
  %B = bitcast x86_mmx %A to <4 x i16>
  ret <4 x i16> %B
}

$ opt -constprop foo.ll -S
define <4 x i16> @test_mmx_const() {
  ret <4 x i16> zeroinitializer
}

(so there should be a negative test for that example in this patch)

You are correct, after reading the code again I'm now convinced that the fault is only related to GEP. I have updated the patch to only affect the GEP case.

Do you think I should still include the negative testcase that you specified above even if the patch now only affect the GEP case?

[spatel]

In D46379#1118803, @Ka-Ka wrote:

Do you think I should still include the negative testcase that you specified above even if the patch now only affect the GEP case?

Yes - because the earlier revision would have caused a bug with no regression test coverage for it. Let's add that to be safe. Also, I'd prefer to use the utils/update_test_checks.py script on all tests because it makes maintenance easier. And as before, if you check in the baseline version of the test using trunk output, that's even better - we want to keep the tests in place even if the patch gets reverted for some reason.

[Ka-Ka]

In D46379#1118995, @spatel wrote:

In D46379#1118803, @Ka-Ka wrote:

Do you think I should still include the negative testcase that you specified above even if the patch now only affect the GEP case?

Yes - because the earlier revision would have caused a bug with no regression test coverage for it. Let's add that to be safe. Also, I'd prefer to use the utils/update_test_checks.py script on all tests because it makes maintenance easier. And as before, if you check in the baseline version of the test using trunk output, that's even better - we want to keep the tests in place even if the patch gets reverted for some reason.

Sure, I include the negative testcase in the file test/Analysis/ConstantFolding/gep-zeroinit-vector.ll

Do you want me to also remove the old testcase test/Transforms/LoopVectorize/X86/constant-fold.ll (that was only created for this issue) as the new one test/Analysis/ConstantFolding/gep-zeroinit-vector.ll replace it?

[spatel]

In D46379#1119010, @Ka-Ka wrote:

Do you want me to also remove the old testcase test/Transforms/LoopVectorize/X86/constant-fold.ll (that was only created for this issue) as the new one test/Analysis/ConstantFolding/gep-zeroinit-vector.ll replace it?

I don't have much of an opinion on that. I assume that's the motivating case. If you think that demonstrates/validates that the bug fix still works on the larger example, it's fine to leave it for added safety.

[Ka-Ka]

Updated testcases according to comments

[spatel]

LGTM.

llvm/trunk/lib/IR/ConstantFold.cpp

if (CE->isCast()) {
       if (unsigned newOpc = foldConstantCastPair(opc, CE, DestTy))
         return ConstantExpr::getCast(newOpc, CE->getOperand(0), DestTy);
     } else if (CE->getOpcode() == Instruction::GetElementPtr &&
                // Do not fold addrspacecast (gep 0, .., 0). It might make the
                // addrspacecast uncanonicalized.
                opc != Instruction::AddrSpaceCast &&
                // Do not fold bitcast (gep) with inrange index, as this loses
                // information.
-               !cast<GEPOperator>(CE)->getInRangeIndex().hasValue()) {
+               !cast<GEPOperator>(CE)->getInRangeIndex().hasValue() &&
+               // Do not fold if the gep type is a vector, as bitcasting
+               // operand 0 of a vector gep will result in a bitcast between
+               // different sizes.
+               !CE->getType()->isVectorTy()) {
       // If all of the indexes in the GEP are null values, there is no pointer
       // adjustment going on.  We might as well cast the source pointer.
       bool isAllNull = true;
       for (unsigned i = 1, e = CE->getNumOperands(); i != e; ++i)
         if (!CE->getOperand(i)->isNullValue()) {
           isAllNull = false;
           break;
         }

llvm/trunk/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 point out faulty bitcast that cast between different sizes.
-; See "bitcast ([1 x %rec8]* @a to <2 x i16*>)" in checks below
+; 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*> bitcast ([1 x %rec8]* @a to <2 x i16*>)
+; CHECK-NEXT:    ret <2 x i16*> <i16* getelementptr inbounds (%rec8, %rec8* extractelement (<2 x %rec8*> getelementptr ([1 x %rec8], [1 x %rec8]* @a, <2 x i64> zeroinitializer, <2 x i64> zeroinitializer), i32 0), i32 0, i32 0), i16* getelementptr inbounds (%rec8, %rec8* extractelement (<2 x %rec8*> getelementptr ([1 x %rec8], [1 x %rec8]* @a, <2 x i64> zeroinitializer, <2 x i64> zeroinitializer), i32 1), i32 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.

llvm/trunk/test/Transforms/LoopVectorize/X86/constant-fold.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -loop-vectorize -S -mtriple=x86_64-- -o - %s | FileCheck %s
 
-; Testcase that point out faulty bitcast that cast between different sizes.
-; See "bitcast ([1 x %rec8]* @a to <2 x i16*>)" in checks below
+; 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
 @b = global [2 x i16*] zeroinitializer
 
 
 define void @f1() {

 ; CHECK-NEXT:    [[BROADCAST_SPLATINSERT:%.*]] = insertelement <2 x i16> undef, i16 [[OFFSET_IDX]], i32 0
 ; CHECK-NEXT:    [[BROADCAST_SPLAT:%.*]] = shufflevector <2 x i16> [[BROADCAST_SPLATINSERT]], <2 x i16> undef, <2 x i32> zeroinitializer
 ; CHECK-NEXT:    [[INDUCTION:%.*]] = add <2 x i16> [[BROADCAST_SPLAT]], <i16 0, i16 1>
 ; CHECK-NEXT:    [[TMP1:%.*]] = add i16 [[OFFSET_IDX]], 0
 ; CHECK-NEXT:    [[TMP2:%.*]] = sext i16 [[TMP1]] to i64
 ; CHECK-NEXT:    [[TMP3:%.*]] = getelementptr [2 x i16*], [2 x i16*]* @b, i16 0, i64 [[TMP2]]
 ; CHECK-NEXT:    [[TMP4:%.*]] = getelementptr i16*, i16** [[TMP3]], i32 0
 ; CHECK-NEXT:    [[TMP5:%.*]] = bitcast i16** [[TMP4]] to <2 x i16*>*
-; CHECK-NEXT:    store <2 x i16*> bitcast ([1 x %rec8]* @a to <2 x i16*>), <2 x i16*>* [[TMP5]], align 8
+; CHECK-NEXT:    store <2 x i16*> <i16* getelementptr inbounds (%rec8, %rec8* extractelement (<2 x %rec8*> getelementptr ([1 x %rec8], [1 x %rec8]* @a, <2 x i16> zeroinitializer, <2 x i64> zeroinitializer), i32 0), i32 0, i32 0), i16* getelementptr inbounds (%rec8, %rec8* extractelement (<2 x %rec8*> getelementptr ([1 x %rec8], [1 x %rec8]* @a, <2 x i16> zeroinitializer, <2 x i64> zeroinitializer), i32 1), i32 0, i32 0)>, <2 x i16*>* [[TMP5]], align 8
 ; CHECK-NEXT:    [[INDEX_NEXT]] = add i32 [[INDEX]], 2
 ; CHECK-NEXT:    [[TMP6:%.*]] = icmp eq i32 [[INDEX_NEXT]], 2
 ; CHECK-NEXT:    br i1 [[TMP6]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop !0
 ; CHECK:       middle.block:
 
 bb1:
   br label %bb2