Page MenuHomePhabricator

[ConstantFold] Turn off expression simplification for vector type
ClosedPublic

Authored by Ka-Ka on May 2 2018, 11:39 PM.

Details

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.

Diff Detail

Event Timeline

Ka-Ka created this revision.May 2 2018, 11:39 PM
Ka-Ka added a comment.May 9 2018, 12:57 AM

Ping!
Anyone up for reviewing this?

lebedev.ri added inline comments.
test/Transforms/LoopVectorize/X86/constant-fold.ll
1
  1. Use utils/update_test_checks.py
  2. Commit the patch with check-lines as of trunk.
  3. Rebase the differential
Ka-Ka updated this revision to Diff 145910.May 9 2018, 7:19 AM
Ka-Ka marked an inline comment as done.

Changed testcase according what was suggested by lebedev.ri

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

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 updated this revision to Diff 149433.Jun 1 2018, 5:21 AM

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 added a comment.Jun 1 2018, 7:08 AM

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 added a comment.Jun 1 2018, 7:23 AM

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 added a comment.Jun 1 2018, 7:32 AM

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 this revision to Diff 149483.Jun 1 2018, 8:36 AM

Updated testcases according to comments

spatel accepted this revision.Jun 1 2018, 9:02 AM

LGTM.

This revision is now accepted and ready to land.Jun 1 2018, 9:02 AM
This revision was automatically updated to reflect the committed changes.