[PATCH] D60506: [CGP] Make ICMP_EQ use CR result of ICMP_S(L|G)T dominators

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

Please reduce the test case only to cases that are changing behaviour. Also, it is probably useful to commit the test case initially as an NFC patch so we can see the actual difference in codegen.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1415
 
+static bool foldICmpWithDominatingICmp(CmpInst *Cmp, const TargetLowering &TLI) {
+  if (TLI.isICMP_EQFoldedWithICMP_ST())
----------------
I think it would be very helpful to comment the code in this function. Specifically, it's good to state the pattern(s) you're looking for. Something like:
```
Block A:
icmp slt i32 %1, 100
...
Block B (with A being the single predecessor):
icmp eq i32 %1, 100 ; Equality is exactly opposite to SGT.
...
```


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1420
+  ICmpInst::Predicate Pred = Cmp->getPredicate();
+  if (Pred != ICmpInst::ICMP_EQ)
+    return false;
----------------
Why do we restrict this to `EQ` rather than also supporting `NE`? Is this just the canonical form and `NE` never shows up?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1423
+
+  Value *X = Cmp->getOperand(0), *Y = Cmp->getOperand(1);
+  const APInt *C;
----------------
More descriptive names would probably aid readability - perhaps `CmpOp1/CmpOp2`.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1425
+  const APInt *C;
+  if (!match(Y, m_APInt(C)))
+    return false;
----------------
Why does this need to be restricted to constant comparisons? Could we not equivalently handle comparisons between two virtual registers?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1433
+      continue;
+    return false;
+  }
----------------
We could probably handle logical operations just as easily, couldn't we?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1452
+
+  ICmpInst::Predicate newPred =
+      DomPred == ICmpInst::ICMP_SGT ? ICmpInst::ICMP_SLT
----------------
Nit: naming convention - if you have other violations of `CamelCase` variable naming, please fix those as well.


================
Comment at: llvm/test/CodeGen/PowerPC/use-cr-result-of-dom-icmp-st.ll:36
+if.end:                                           ; preds = %entry
+  %cmp1 = icmp slt i64 %a, %b
+  %mul = select i1 %cmp1, i64 1, i64 %b
----------------
This is already in the form you want it to be in, so it doesn't really test anything this pass does.


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

https://reviews.llvm.org/D60506