<div dir="ltr"><div class="gmail_extra">Now that the bots are happy, I wanted to do a bit of code review on the test cases here.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_quote">On Thu, May 29, 2014 at 1:29 PM, Louis Gerbarg <span dir="ltr"><<a href="mailto:lgg@apple.com" target="_blank">lgg@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":6ap" class="a3s" style="overflow:hidden">Added: llvm/trunk/test/Transforms/InstCombine/gepphigep.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/gepphigep.ll?rev=209843&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/gepphigep.ll?rev=209843&view=auto</a><br>

==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/gepphigep.ll (added)<br>
+++ llvm/trunk/test/Transforms/InstCombine/gepphigep.ll Thu May 29 15:29:47 2014<br>
@@ -0,0 +1,56 @@<br>
+; RUN: opt -instcombine -S  < %s | FileCheck %s<br>
+<br>
+%struct1 = type { %struct2*, i32, i32, i32 }<br>
+%struct2 = type { i32, i32 }<br>
+<br>
+define i32 @test1(%struct1* %dm, i1 %tmp4, i64 %tmp9, i64 %tmp19) {<br>
+bb:<br>
+  %tmp = getelementptr inbounds %struct1* %dm, i64 0, i32 0<br>
+  %tmp1 = load %struct2** %tmp, align 8<br>
+  br i1 %tmp4, label %bb1, label %bb2<br>
+<br>
+bb1:<br>
+  %tmp10 = getelementptr inbounds %struct2* %tmp1, i64 %tmp9<br>
+  %tmp11 = getelementptr inbounds %struct2* %tmp10, i64 0, i32 0<br></div></blockquote><div><br></div><div>Why the extra (dead) GEP? It isn't used around a PHI node...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div id=":6ap" class="a3s" style="overflow:hidden">
+  store i32 0, i32* %tmp11, align 4<br>
+  br label %bb3<br>
+<br>
+bb2:<br>
+  %tmp20 = getelementptr inbounds %struct2* %tmp1, i64 %tmp19<br>
+  %tmp21 = getelementptr inbounds %struct2* %tmp20, i64 0, i32 0<br></div></blockquote><div><br></div><div>Ditto...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div id=":6ap" class="a3s" style="overflow:hidden">
+  store i32 0, i32* %tmp21, align 4<br>
+  br label %bb3<br>
+<br>
+bb3:<br>
+  %phi = phi %struct2* [ %tmp10, %bb1 ], [ %tmp20, %bb2 ]<br>
+  %tmp24 = getelementptr inbounds %struct2* %phi, i64 0, i32 1<br>
+  %tmp25 = load i32* %tmp24, align 4<br>
+  ret i32 %tmp25<br>
+<br>
+; CHECK-LABEL: @test1(<br>
+; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp9, i32 0<br>
+; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp19, i32 0<br></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":6ap" class="a3s" style="overflow:hidden">
+; CHECK: %[[PHI:[0-9A-Za-z]+]] = phi i64 [ %tmp9, %bb1 ], [ %tmp19, %bb2 ]<br>
+; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %[[PHI]], i32 1<br>
+<br>
+}<br>
+<br>
+define i32 @test2(%struct1* %dm, i1 %tmp4, i64 %tmp9, i64 %tmp19) {<br>
+bb:<br>
+  %tmp = getelementptr inbounds %struct1* %dm, i64 0, i32 0<br>
+  %tmp1 = load %struct2** %tmp, align 8<br>
+  %tmp10 = getelementptr inbounds %struct2* %tmp1, i64 %tmp9<br>
+  %tmp11 = getelementptr inbounds %struct2* %tmp10, i64 0, i32 0<br>
+  store i32 0, i32* %tmp11, align 4<br>
+  %tmp20 = getelementptr inbounds %struct2* %tmp1, i64 %tmp19<br>
+  %tmp21 = getelementptr inbounds %struct2* %tmp20, i64 0, i32 0<br>
+  store i32 0, i32* %tmp21, align 4<br>
+  %tmp24 = getelementptr inbounds %struct2* %tmp10, i64 0, i32 1<br>
+  %tmp25 = load i32* %tmp24, align 4<br>
+  ret i32 %tmp25<br>
+<br>
+; CHECK-LABEL: @test2(<br>
+; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp9, i32 0<br>
+; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp19, i32 0<br>
+; CHECK: getelementptr inbounds %struct2* %tmp1, i64 %tmp9, i32 1 </div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":6ap" class="a3s" style="overflow:hidden">

+}</div></blockquote><div><br></div><div>What is the second test checking? There are no PHI nodes here, so it is really opaque what you're even testing.</div><div><br></div><div>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:</div>
<div><br></div><div>1) Negative tests for when we reach a variable index into a struct, clearly commenting what it is checking.</div><div>2) Negative tests for when the PHI node has two GEP operands but the operands have different types.</div>
<div>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).</div><div>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.</div>
<div>5) Positive tests for when the variable index is inside a an array inside of a struct<br></div><div>6) Positive tests for when a PHI operand or a GEP operand is undef</div><div>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.</div>
<div>8) Tests for vector-GEPs</div><div><br></div><div>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?</div>
<div><br></div><div>Thanks!</div><div>-Chandler</div></div><br><br></div></div>