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

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames added a comment.

In D100884#2705497 <https://reviews.llvm.org/D100884#2705497>, @nikic wrote:

> 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);

I think you are correct here.   I had to stare at this a bit, but I'm not seeing any problems with it.

I'll go back to the Value approach and add tests for these cases next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100884