[PATCH] D100884: Generalize getInvertibleOperand to support mismatched phi operand order

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D100884#2703133 <https://reviews.llvm.org/D100884#2703133>, @reames wrote:

> In D100884#2703064 <https://reviews.llvm.org/D100884#2703064>, @nikic wrote:
>
>> I don't think this is particularly valuable in terms of optimization power, but I do think the interface makes a bit more sense that way. Though I would suggest to go one step further and return `std::pair<Value *, Value *>`. That is, for a pair of input values, it returns a pair of output values that preserve the equality relationship. This should make the phi case particularly nice, because you can simply return `{Start1, Start2}` rather than checking for different possibilities of operand orders.
>
> I played with this, but it turns out a bit more ugly than it sounds.  It requires a bunch of duplicated boilerplate for the non-phi return points, and more importantly, greatly complicates the check to see that the recurrence is the LHS of the operation.  I really didn't care for the resulting code structure.

Right, the LHS check would become rather ugly ... but upon further consideration, why do we need that check at all? I believe the logic holds either way. While a cycle over `sub 0, %A` and `sub 0, %B` is certainly odd, the reasoning (sequence of invertible operations) remains correct.

I believe it would be correct to reduce the phi case to just:

  auto Values = getInvertibleOperand(cast<Operator>(BO1), cast<Operator>(BO2));
  if (!Values)
    break;
  assert(Values->first == PN1 && Values->second == PN2);
  return std::make_pair(Start1, Start2);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100884/new/

https://reviews.llvm.org/D100884