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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some thoughts



================
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."));
----------------
What's the plan with defaulting to `false`?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1440
+  ICmpInst::Predicate Pred = Cmp->getPredicate();
+  if (Pred != ICmpInst::ICMP_EQ)
+    return false;
----------------
What about `ICmpInst::ICMP_NE`?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1443
+
+  // If icmp eq is not only used by BranchInst or SelectInst, converting it to
+  // icmp slt/sgt would introduce more redundant LLVM IR.
----------------
s/not only used by/has users other than/ s/or/and/


================
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:
> s/not only used by/has users other than/ s/or/and/
You want to use `canFreelyInvertAllUsersOf()` i think.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1464-1465
+    return false;
+  if (CmpBB != FalseBB)
+    return false;
+
----------------
Is this guaranteed, or are some commutative cases being missed?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1474-1475
+
+  ICmpInst::Predicate NewPred =
+      DomPred == ICmpInst::ICMP_SGT ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_SGT;
+
----------------
ICmpInst::Predicate NewPred = CmpInst::getSwappedPredicate(DomPred);


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1485-1489
+      Value *TrueValue = SI->getTrueValue();
+      Value *FalseValue = SI->getFalseValue();
+      SI->setTrueValue(FalseValue);
+      SI->setFalseValue(TrueValue);
+      SI->swapProfMetadata();
----------------
SI->swapValues();


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

https://reviews.llvm.org/D60506