[PATCH] D112060: [NARY-REASSOCIATE] Fix infinite recursion optimizing min\max

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:615
             continue;
-          // Transform 'I = (A op RHS) op B' 'I = (B op RHS) op A' on the second
+          // Transform 'I = (A op C) op B' to 'I = (B op C) op A' on the second
           // iteration.
----------------
ebrevnov wrote:
> ebrevnov wrote:
> > nikic wrote:
> > > spatel wrote:
> > > > This code comment doesn't match the code?
> > > Something that also confuses me about this code is why it performs a loop with two iterations over `j`, and then has completely separate behavior for them. Wouldn't this whole loop be equivalent to the following?
> > > ```
> > > if (BExpr != CExpr) {
> > >   std::swap(BExpr, CExpr);
> > >   std::swap(B, C);
> > > }
> > > if (AExpr != CExpr) {
> > >   std::swap(AExpr, CExpr);
> > >   std::swap(A, C);
> > > }
> > > ```
> > > Or if we allow some redundant swaps just:
> > > ```
> > > std::swap(BExpr, CExpr);
> > > std::swap(B, C);
> > > std::swap(AExpr, CExpr);
> > > std::swap(A, C);
> > > ```
> > > Which reorders `A B C` into `B C A`.
> > > 
> > > Though as all of these variables are under your control I think you could just directly match them into the right variables rather than swapping them after the fact.
> > > 
> > > Am I missing something here?
> > > This code comment doesn't match the code?
> > 
> > It does matches the code but not easy to follow. The trick is that pointers named A, B, and C refers to different expressions on each iteration. For example, after the first swap B points to what C used to point.... I agree all that looks confusing ... I will try to restructure the code to make it simpler for understanding.
> > 
> > Something that also confuses me about this code is why it performs a loop with two iterations over `j`, and then has completely separate behavior for them. Wouldn't this whole loop be equivalent to the following?
> > ```
> > if (BExpr != CExpr) {
> >   std::swap(BExpr, CExpr);
> >   std::swap(B, C);
> > }
> > if (AExpr != CExpr) {
> >   std::swap(AExpr, CExpr);
> >   std::swap(A, C);
> > }
> > ```
> > Or if we allow some redundant swaps just:
> > ```
> > std::swap(BExpr, CExpr);
> > std::swap(B, C);
> > std::swap(AExpr, CExpr);
> > std::swap(A, C);
> > ```
> > Which reorders `A B C` into `B C A`.
> > 
> > Though as all of these variables are under your control I think you could just directly match them into the right variables rather than swapping them after the fact.
> > 
> > Am I missing something here?
> 
> No, this is not semantically equivalent code. The idea is to try two combinations: (AopC)opB and then (BopC)opA. That's why the loop of two iterations.
> I agree all that look confusing...I will try to restructure the code to make it simpler for understanding.
> No, this is not semantically equivalent code. The idea is to try two combinations: (AopC)opB and then (BopC)opA. That's why the loop of two iterations.

Uh yeah, I misread the code. For some reason I thought the `j` loop ends after the swaps, but there's more code after it :)

Maybe a clearer way to do this is to have a helper function with the actual logic, and then call it twice with arguments `A, C, B` and `B, C, A`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112060