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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D45842#1081028, @opaparo wrote:

> What are your plans for deeper associate-operand trees, such as in the tests I've suggested at https://reviews.llvm.org/D41574? Will you support such cases?


This patch is independent of https://reviews.llvm.org/D41574 from what I see. Ie, that patch makes no difference on any of these tests. Could it be enhanced to catch these?

If we want to do something like https://reviews.llvm.org/D41574 (provide stronger sorting of the expression tree based on opcode/operand) here in the existing -reassociate, then an addition to ReassociateExpression() like this could be used:

  for (unsigned i = 0; i < Ops.size() - 1; ++i) {
    for (unsigned j = i + 1; j < Ops.size(); ++j) {
      BinaryOperator *B0, *B1;
      if (!match(Ops[i].Op, m_BinOp(B0)))
        continue;
      if (match(Ops[j].Op, m_BinOp(B1)) && B0->getOpcode() < B1->getOpcode())
        continue;
      if (B0->getOpcode() == B1->getOpcode()) {
        const APInt *B0C, *B1C;
        if (match(B0->getOperand(1), m_APInt(B0C)) && match(B1->getOperand(1), m_APInt(B1C)))
          if (B0C->ule(*B1C))
            continue;
      }
      std::swap(Ops[i], Ops[j]);
    }
  }

That creates some of the factoring folds that we want, but it won't reduce as far as shown in the other patch. I'm not sure if that's a limitation of this pass or if I just botched the code (and this is untested, so may not be correct).


https://reviews.llvm.org/D45842