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

[James Molloy]

jmolloy added a subscriber: jmolloy.
jmolloy requested changes to this revision.
jmolloy added a reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Jakub,

Thanks for working on this! I've got a bunch of comments from this first round of review.

Cheers,

James


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:784
@@ -783,1 +783,3 @@
 
+// If PHI node has two edges and the PHI node is used in instructions like
+// add, sub, mul, div, shifts; if one of incoming values is a constant
----------------
"if *a* PHI node"

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:787
@@ +786,3 @@
+// that makes the instruction and identity operation, we can move this
+// instruction into one of the basic blocks.
+// Because of such a transformation, the identity operation won't be
----------------
"we can hoist this instruction into only one of the incoming blocks"

The use of "hoist" is important, I think.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:795
@@ +794,3 @@
+                                                 InstCombiner &IC) {
+  if (!PNUser.isBinaryOp())
+    return nullptr;
----------------
There are a lot of ternary operators (x ? y : z) going on in this function, and it's a bit difficult to assert that they're all correct.

What I'd like to see is some ascii-art of the situation, with each of the nodes having a name. Then variables refer to those names later, which means I don't get confused which node OpOperandIdx refers to!

    C        V
      \      /
       Phi
    X  |
     \  |
       Op

Something like that? ^

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:809
@@ +808,3 @@
+    return nullptr;
+  case Instruction::Add: {
+    if (!C.isZeroValue())
----------------
Remove braces and put break on its own line.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:817
@@ +816,3 @@
+  case Instruction::LShr:
+  case Instruction::AShr: {
+    if (!C.isZeroValue() || (OpOperandIdx == 1))
----------------
Same as line 809, and for the rest.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:818
@@ +817,3 @@
+  case Instruction::AShr: {
+    if (!C.isZeroValue() || (OpOperandIdx == 1))
+      return nullptr;
----------------
No parens () around the second expression.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:838
@@ +837,3 @@
+  if (const auto *Incoming = dyn_cast<Instruction>(&IncomingVal))
+    if (!IC.getDominatorTree()->dominates(Incoming, Terminator))
+      return nullptr;
----------------
You need to handle the case where dyn_cast returns nullptr here.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:843
@@ +842,3 @@
+  // as an incoming value of the PHI node.
+  if (const auto *Operand =
+          dyn_cast<Instruction>(PNUser.getOperand(OpOperandIdx)))
----------------
Same as line 838.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:846
@@ +845,3 @@
+    if (!IC.getDominatorTree()->dominates(Operand, Terminator) ||
+        !IC.getDominatorTree()->dominates(Operand, &PN))
+      return nullptr;
----------------
I think this second check is unnecessary. As the terminator leads to the block with the PHI in, any instruction that dominates the terminator must also dominate PN.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:856
@@ +855,3 @@
+  for (const auto &U : PNUser.uses())
+    if (IC.getDominatorTree()->dominates(Terminator, U))
+      return nullptr;
----------------
What is this doing? What situation can cause whatever this is checking against?

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:865
@@ +864,3 @@
+  IRBuilder<> Builder(Terminator);
+  auto *NewInst = cast<BinaryOperator>(Builder.CreateBinOp(
+      (Instruction::BinaryOps)PNUser.getOpcode(), LHS, RHS));
----------------
Better to use a cast<> rather than a C-style cast for PNUser.getOpcode(); that's safer:

    auto *NewInst = Builder.CreateBinOp(cast<BinaryOperator>(PNUser).getOpcode(), LHS, RHS);
    cast<BinaryOperator>(NewInst)->copyIRFlags(&PNUser);


Repository:
  rL LLVM

http://reviews.llvm.org/D11648