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

[Yi-Hong Lyu via Phabricator via llvm-commits]

Yi-Hong.Lyu marked 2 inline comments as done.
Yi-Hong.Lyu added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:226
+static cl::opt<bool> EnableICMP_EQToICMP_ST(
+    "cgp-icmp-eq2icmp-st", cl::Hidden, cl::init(false),
+    cl::desc("Enable ICMP_EQ to ICMP_S(L|G)T conversion."));
----------------
lebedev.ri wrote:
> What's the plan with defaulting to `false`?
We indeed know it remove comparison on PowerPC but we don't know whether it has negative impacts for other targets. We disable it by default and let other target to decide whether to turn it on.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1440
+  ICmpInst::Predicate Pred = Cmp->getPredicate();
+  if (Pred != ICmpInst::ICMP_EQ)
+    return false;
----------------
lebedev.ri wrote:
> What about `ICmpInst::ICMP_NE`?
Please see discussion in https://reviews.llvm.org/D60506?id=194492#inline-551643. In short, `EQ` is canonical form and `NE` never shows up. 


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1443-1452
+  // If icmp eq is not only used by BranchInst or SelectInst, converting it to
+  // icmp slt/sgt would introduce more redundant LLVM IR.
+  for (User *U : Cmp->users()) {
+    if (isa<BranchInst>(U) && cast<BranchInst>(U)->isConditional())
+      continue;
+    if (isa<SelectInst>(U) && cast<SelectInst>(U)->getCondition() == Cmp)
+      continue;
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > s/not only used by/has users other than/ s/or/and/
> You want to use `canFreelyInvertAllUsersOf()` i think.
`canFreelyInvertAllUsersOf` is internal to `llvm/lib/Transforms/InstCombine/InstCombineInternal.h` that I cannot include and use. Should I submit a NFC patch to move it to some header so I can invoke it?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1464-1465
+    return false;
+  if (CmpBB != FalseBB)
+    return false;
+
----------------
lebedev.ri wrote:
> Is this guaranteed, or are some commutative cases being missed?
It is guaranteed and I am not aware of any commutative cases being missed. Without this check, some pattern like
```
///   DomCond = icmp sgt/slt CmpOp0, CmpOp1 (might not be in DomBB)
///   ...
/// DomBB:
///   ...
///   br DomCond, CmpBB, FalseBB
/// CmpBB: (with DomBB being the single predecessor)
///   ...
///   Cmp = icmp eq CmpOp0, CmpOp1

```
would continue execution. Apparently the `Cmp = icmp eq CmpOp0, CmpOp1` would be always false (i.e., redundant) and it is not the pattern we are looking for.


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

https://reviews.llvm.org/D60506