[PATCH] D156238: [InstCombine] Generalize foldICmpWithMinMax

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:5003
+    //   X == Z       min(X, Y) == Z       X <= Y
+    //   X == Z       max(X, Y) == Z       X >= Y
+    //   X != Z       min(X, Y) == Z   X > Y && Y == Z // nofold
----------------
Think you mean `Y == Z` here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:5005
+    //   X != Z       min(X, Y) == Z   X > Y && Y == Z // nofold
+    //   X != Z       max(X, Y) == Z   X < Y && Y == Z // nofold
+    //   X == Z       min(X, Y) != Z       X > Y
----------------
Think you mean `Y != Z`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:5007
+    //   X == Z       min(X, Y) != Z       X > Y
+    //   X == Z       max(X, Y) != Z       X < Y
+    //   X != Z       min(X, Y) != Z   X <= Y || Y != Z // nofold
----------------
same thing and likewise below.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:5044
+    //    X < Z       min(X, Y) >= Z      false
+    //    X < Z       max(X, Y) >= Z      Y >= Z
+
----------------
Same issue with the fact column as above.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:5045
+    //    X < Z       max(X, Y) >= Z      Y >= Z
+
+    auto FoldIntoConstant = [&](bool Value) {
----------------
Note on the comments. These are large blocks that are easy to get lost in.

I would suggest reformatting as follows.

1) Format as:
`Expr Fact Result`. The expression is really what allows us to match the comment to a predicate / the codes you are transforming.

2) Split the comments so instead of giant blocks, put the relevenat `Expr` below each case in the switch statemnt i.e

```
case SLT:
case ULT:
// Expr Fact Result where expr is LT variant.
case SLE:
case ULE:
// Expr Fact Result where expr is LE variant.
...
```

Likewise for the eq/ne cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156238