[PATCH] D45842: [Reassociate] swap binop operands to increase factoring potential

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Overall, this //seems// to make sense to me, but the comments seem misdirecting.



================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2138
+
+  // Canonicalize a matching binop to B0 and a non-matching binop to B1.
+  Instruction::BinaryOps TopOpc = B.getOpcode();
----------------
And by "matching" you really mean "with the same type as the parent binop"


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2141
+  if (B0->getOpcode() != TopOpc) {
+    if (B1->getOpcode() != TopOpc || !B1->hasOneUse())
+      return;
----------------
Why can't we swap if `B1` has more than one use?
And why is it not a problem for `B0`? (and positive test for this multi-use would be awesome)


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2147
+
+  // If both operands already have the same opcode, nothing to do.
+  Instruction::BinaryOps OtherOpc = B1->getOpcode();
----------------
// If B0 is still not matching, or both operands already have the same opcode, nothing to do.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2152-2153
+
+  // Canonicalize a binop that matches B1 to B00 (operand 0 of B0) and a value
+  // that does not match B1 to V01.
+  bool SwappedB0 = false;
----------------
I find this comment not strictly true.
We only check that `B00->getOpcode() != OtherOpc`, we don't check `B01`.
Also s/V01/B01/


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2157
+  if (!match(B0->getOperand(0), m_BinOp(B00)) || B00->getOpcode() != OtherOpc) {
+    B0->swapOperands();
+    SwappedB0 = true;
----------------
And here we don't care about one-use?


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2161
+
+  // B00 must match B1 and not match V01.
+  if (match(B0->getOperand(0), m_BinOp(B00)) && B00->getOpcode() == OtherOpc) {
----------------
B01 ?


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2163-2165
+    Value *V01 = B0->getOperand(1);
+    BinaryOperator *B01;
+    if (!match(V01, m_BinOp(B01)) || B01->getOpcode() != OtherOpc) {
----------------
I see, so `V01` and `B01` are actually the same thing, but with different type.
Would it be better to condense it into one `if`, like:
```
BinaryOperator *B01;
if (( match(B0->getOperand(0), m_BinOp(B00)) && B00->getOpcode() == OtherOpc) && 
    (!match(B0->getOperand(1), m_BinOp(B01)) || B01->getOpcode() != OtherOpc)) {
  Value *V01 = B0->getOperand(1);
```
?


================
Comment at: test/Transforms/Reassociate/matching-binops.ll:186
 
 ; Negative test - multi-use
 
----------------
The logic behind one-use restrictions in this case is not clear to me,
so i'd personally prefer to have more one-use tests, where one-use is
the only thing that is preventing the reassociation.


https://reviews.llvm.org/D45842