[llvm] r209843 - Add support for combining GEPs across PHI nodes

[Chandler Carruth]

Now that the bots are happy, I wanted to do a bit of code review on the
test cases here.

On Thu, May 29, 2014 at 1:29 PM, Louis Gerbarg <lgg at apple.com> wrote:

> Added: llvm/trunk/test/Transforms/InstCombine/gepphigep.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/gepphigep.ll?rev=209843&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/gepphigep.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/gepphigep.ll Thu May 29
> 15:29:47 2014
> @@ -0,0 +1,56 @@
> +; RUN: opt -instcombine -S  < %s | FileCheck %s
> +
> +%struct1 = type { %struct2*, i32, i32, i32 }
> +%struct2 = type { i32, i32 }
> +
> +define i32 @test1(%struct1* %dm, i1 %tmp4, i64 %tmp9, i64 %tmp19) {
> +bb:
> +  %tmp = getelementptr inbounds %struct1* %dm, i64 0, i32 0
> +  %tmp1 = load %struct2** %tmp, align 8
> +  br i1 %tmp4, label %bb1, label %bb2
> +
> +bb1:
> +  %tmp10 = getelementptr inbounds %struct2* %tmp1, i64 %tmp9
> +  %tmp11 = getelementptr inbounds %struct2* %tmp10, i64 0, i32 0
>

Why the extra (dead) GEP? It isn't used around a PHI node...


> +  store i32 0, i32* %tmp11, align 4
> +  br label %bb3
> +
> +bb2:
> +  %tmp20 = getelementptr inbounds %struct2* %tmp1, i64 %tmp19
> +  %tmp21 = getelementptr inbounds %struct2* %tmp20, i64 0, i32 0
>

Ditto...


> +  store i32 0, i32* %tmp21, align 4
> +  br label %bb3
> +
> +bb3:
> +  %phi = phi %struct2* [ %tmp10, %bb1 ], [ %tmp20, %bb2 ]
> +  %tmp24 = getelementptr inbounds %struct2* %phi, i64 0, i32 1
> +  %tmp25 = load i32* %tmp24, align 4
> +  ret i32 %tmp25
> +
> +; CHECK-LABEL: @test1(
> +; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp9, i32 0
> +; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp19, i32 0
>

And here we check that the classical instcombine applies to the dead GEPs.
In general, I would avoid adding code and assertions about unrelated
transforms and just keep the test focused on the transform you have in mind.


> +; CHECK: %[[PHI:[0-9A-Za-z]+]] = phi i64 [ %tmp9, %bb1 ], [ %tmp19, %bb2 ]
> +; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %[[PHI]], i32 1
> +
> +}
> +
> +define i32 @test2(%struct1* %dm, i1 %tmp4, i64 %tmp9, i64 %tmp19) {
> +bb:
> +  %tmp = getelementptr inbounds %struct1* %dm, i64 0, i32 0
> +  %tmp1 = load %struct2** %tmp, align 8
> +  %tmp10 = getelementptr inbounds %struct2* %tmp1, i64 %tmp9
> +  %tmp11 = getelementptr inbounds %struct2* %tmp10, i64 0, i32 0
> +  store i32 0, i32* %tmp11, align 4
> +  %tmp20 = getelementptr inbounds %struct2* %tmp1, i64 %tmp19
> +  %tmp21 = getelementptr inbounds %struct2* %tmp20, i64 0, i32 0
> +  store i32 0, i32* %tmp21, align 4
> +  %tmp24 = getelementptr inbounds %struct2* %tmp10, i64 0, i32 1
> +  %tmp25 = load i32* %tmp24, align 4
> +  ret i32 %tmp25
> +
> +; CHECK-LABEL: @test2(
> +; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp9, i32 0
> +; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp19, i32 0
> +; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp9, i32 1
>
+}
>

What is the second test checking? There are no PHI nodes here, so it is
really opaque what you're even testing.

Also, it doesn't look like you've included any negative tests (unless test2
is supposed to be such a test). My suspicion is that the bugs in the patch
might have been easier to shake out with some. Here is how I would have
broken down the testing:

1) Negative tests for when we reach a variable index into a struct, clearly
commenting what it is checking.
2) Negative tests for when the PHI node has two GEP operands but the
operands have different types.
3) Negative tests for when the PHI node has two GEP operands *almost*
identical types (the differing types are hidden way down in the GEP).
4) Negative tests for when the PHI node has two GEP operands with different
numbers of indices *but the same* types. This is tricky, but you can do it
with differently indexed structs.
5) Positive tests for when the variable index is inside a an array inside
of a struct
6) Positive tests for when a PHI operand or a GEP operand is undef
7) Tests (positive and negative?) for GEPs over vectors. I despise the
existence of these in the IR but we should make sure they work.
8) Tests for vector-GEPs

If I understand the code correctly at least #5 and #6 won't work without
fixes to the code itself. Could you look into adding (something along the
lines of) these cases?

Thanks!
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140529/a30aca43/attachment.html>