[PATCH] D11648: InstCombinePHI: Partial simplification of identity operations

[James Molloy]

jmolloy added a comment.

Hi Jakub,

Thanks for making those changes. I just have one more tranche of changes to request; they're all to do with the testcase.

Cheers,

James


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:846
@@ +845,3 @@
+  auto *Terminator = BB->getTerminator();
+
+  if (const auto *Incoming = dyn_cast<Instruction>(&IncomingVal))
----------------
Okay, after chatting with Jakub offline, the simpler case here is a single-block loop where "Operand" is inside the loop. It'll dominate the teminator, but not the PHI node because it is between them.

================
Comment at: test/Transforms/InstCombine/phi.ll:636
@@ +635,3 @@
+; CHECK-LABEL: @test28(
+; CHECK add nsw
+; CHECK: v.addr.0
----------------
Your CHECKs here need to be more explicit. I don't think they're tight enough now to validate that the test does what it's supposed to.

You should check that the if.end block contains only a PHI and a ret:

    // CHECK: if.end:
    // CHECK-NEXT: phi [ %{{.*}}, %if.else ], [ %v.addr.0, %entry ]
    // CHECK-NEXT: ret

================
Comment at: test/Transforms/InstCombine/phi.ll:655
@@ +654,3 @@
+; CHECK-LABEL: @test29(
+; CHECK: v.addr.0
+; CHECK-NEXT: ret
----------------
Same for all the rest of these; be more specific.


Repository:
  rL LLVM

http://reviews.llvm.org/D11648