[PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Nice, pretty much there!



================
Comment at: lib/Analysis/InstructionSimplify.cpp:73-74
+/// We want to turn:
+///   (select (icmp sgt x, C), lshr (X, Y), ashr (X, Y)); C >= -1
+///   (select (icmp slt x, C), ashr (X, Y), lshr (X, Y)); C >= 0
+/// into:
----------------
```
///   (select (icmp sgt x, C), lshr (X, Y), ashr (X, Y)); iff C s>= -1
///   (select (icmp slt x, C), ashr (X, Y), lshr (X, Y)); iff C s>= 0
```


================
Comment at: lib/Analysis/InstructionSimplify.cpp:77-78
+///   ashr (X, Y)
+/// if both of the shifts either have or have no exact; or only
+/// lshr is exact.
+static Value *foldSelectICmpLshrAshr(const ICmpInst::Predicate Pred,
----------------
I think it would be less confusing to reword as 
```
/// iff either `ashr` is not `exact`, or `lshr` is 'exact' too.
```



================
Comment at: lib/Analysis/InstructionSimplify.cpp:86
+      (Pred == ICmpInst::ICMP_SLT && match(CmpRHS, m_APInt(C)) && C->sge(0))) {
+    if (Pred == ICmpInst::ICMP_SLT)
+      std::swap(TrueVal, FalseVal);
----------------
```
/// Canonicalize so that `ashr` is in FalseVal.
```


================
Comment at: lib/Analysis/InstructionSimplify.cpp:92
+        match(CmpLHS, m_Specific(X))) {
+      if (!cast<BinaryOperator>(TrueVal)->isExact() &&
+          cast<BinaryOperator>(FalseVal)->isExact())
----------------
```
/// If either the `ashr` is non-`exact`, or `lshr` is exact too, then we can just return the `ashr` instruction.
```
i think it may be less confusing to flip the `if` accordingly.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:97-99
+  }
+
+  return nullptr;
----------------
early return?


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:539
+///   ashr (X, Y)
+/// iff only ashr is exact.
+static Value *foldSelectICmpLshrAshr(const ICmpInst *IC, Value *TrueVal,
----------------
I don't think we want to make any such restrictions.
```
/// into:
///   ashr (X, Y)
/// It is assumed that InstSimplify has already run.
```



================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:547-565
+  Value *X, *Y;
+  const APInt *C;
+  if ((Pred == ICmpInst::ICMP_SGT && match(CmpRHS, m_APInt(C)) && C->sge(-1)) ||
+      (Pred == ICmpInst::ICMP_SLT && match(CmpRHS, m_APInt(C)) && C->sge(0))) {
+    if (Pred == ICmpInst::ICMP_SLT)
+      std::swap(TrueVal, FalseVal);
+
----------------
Same comments as to instsimplify function - comments, early return


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:557-558
+        match(CmpLHS, m_Specific(X)) &&
+        (!cast<BinaryOperator>(TrueVal)->isExact() &&
+         cast<BinaryOperator>(FalseVal)->isExact())) {
+      cast<BinaryOperator>(FalseVal)->setIsExact(false);
----------------
No need for this check.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:559
+         cast<BinaryOperator>(FalseVal)->isExact())) {
+      cast<BinaryOperator>(FalseVal)->setIsExact(false);
+      return FalseVal;
----------------
I'd really recommend to create a new instruction.
It will result in same source line count, but is that much more transparent to the pass,
and is e.g. visible in `-debug` output.


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

https://reviews.llvm.org/D64285