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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov 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.
----------------
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.



================
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:
> 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.


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:642
+        SmallVector<const SCEV *, 2> Ops2{SE->getUnknown(C),
+                                          SE->getUnknown(R1MinMax)};
         const SCEV *R2Expr = SE->getMinMaxExpr(SCEVType, Ops2);
----------------
nikic wrote:
> You can pass these directly, no need to create SmallVector for an ArrayRef.
> You can pass these directly, no need to create SmallVector for an ArrayRef.

Thanks. Will fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112060