[PATCH] D24147: Lower consecutive select instructions correctly.

[Vedant Kumar via llvm-commits]

vsk added a subscriber: vsk.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4545
@@ +4544,3 @@
+/// Return true if SI2 depends on any select instruction between SI1 and SI2
+static bool isDependentSI(SelectInst *SI1, SelectInst *SI2) {
+  if (SI1 == SI2)
----------------
Assert SI1->getParent() == SI2->getParent()?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4549
@@ +4548,3 @@
+  for (BasicBlock::iterator It = BasicBlock::iterator(SI1);
+       It !=SI1->getParent()->end(); ++It) {
+    SelectInst *SI = dyn_cast<SelectInst>(&*It);
----------------
Use 'auto It', a const_iterator, and clang-format?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4553
@@ +4552,3 @@
+      return false;
+    if (SI == SI2->getTrueValue() || SI == SI2->getFalseValue())
+      return true;
----------------
Is SI2 not dependent on SI1 if, e.g `SI2->getTrueValue() == (add SI 1)`? I'm fuzzy on what 'dependent' means in this context, sorry!

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4556
@@ +4555,3 @@
+  }
+  assert(0 && "SI1 does not dominate SI2 in the same BB");
+  return true;
----------------
This would be better off as an llvm_unreachable; that should let you remove the return.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4571
@@ +4570,3 @@
+      ASI.push_back(I);
+      if (isDependentSI(SI, I))
+        ret = false;
----------------
I'm worried that this is quadratic in the number of SelectInsts in a BB. Is there a way to perform the isDependent check by looking at the users of a SelectInst instead of traversing the BB?

================
Comment at: test/CodeGen/X86/codegen-prepare-select.ll:8
@@ +7,3 @@
+; CHECK: %select.false
+; CHECK-NOT: %select.false2
+
----------------
Please move these checks closer to the IR which produces the expected assembly.

Similarly, please move the NOT tests closer to the IR which used to produce the corresponding assembly.

================
Comment at: test/CodeGen/X86/codegen-prepare-select.ll:40
@@ +39,3 @@
+}
+
+!1 = !{!2, !2, i64 0}
----------------
It would help me follow along if you could reduce this test a bit more. E.g, besides the branch_weights, I'm not sure if any of the metadata here is really needed. Similarly, it's not clear if/why some of the operations in the loop are really needed (mul, ashr, sext). If they really are needed, it would help to have a small explanatory comment.


https://reviews.llvm.org/D24147