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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

LGTM with some minor nits related to comments/usage addressed. I personally don't have an issue with you addressing this on the commit, but give it until early next week in case someone else speaks up regarding wanting another review.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1446
+  for (User *U : Cmp->users()) {
+    if (isa<BranchInst>(U) && cast<BranchInst>(U)->isConditional())
+      continue;
----------------
Nit: the sequence `isa<>; cast<>` is discouraged. The preferred way is to `dyn_cast` and the subsequent checks are done if the return value is non-null.
Also, is the check for `isConditional()` actually needed? Can an unconditional branch use the result of a compare?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1460
+
+  Value *DomCond;
+  BasicBlock *TrueBB, *FalseBB;
----------------
I think a comment before this section along the following lines would help readability:
```
// We want to ensure that the only way control gets to the comparison
// of interest is that a less/greater than comparison on the same operands
// is false.
```


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1474
+
+  for (User *U : Cmp->users()) {
+    if (auto *BI = dyn_cast<BranchInst>(U)) {
----------------
Also a comment here would be nice. Something like:
```
// Convert the equality comparison to the opposite of the dominating
// comparison and swap the direction for all branch/select users.
// We have conceptually converted:
// Res = (a < b) ? <LT_RES> : (a == b) ? <EQ_RES> : <GT_RES>;
// to
// Res = (a < b) ? <LT_RES> : (a > b)  ? <GT_RES> : <EQ_RES>;
// And similarly for branches.
```


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

https://reviews.llvm.org/D60506