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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

The fix itself (using SCEVUnknown) looks good to me, though I agree that the whole implementation is pretty odd and could be cleaned up in a followup change.



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


================
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);
----------------
You can pass these directly, no need to create SmallVector for an ArrayRef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112060