[PATCH] D15496: [InstCombine] Identify partial bitreverses

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

I have a couple of (possibly naive) questions.

I'm not familiar with this code, so while I can help review it, I think the final LGTM should come from someone else more familiar with this code.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1584
@@ -1583,3 +1583,3 @@
 ///
 /// This function returns true if the match was unsuccessful and false if so.
 /// On entry to the function the "OverallLeftShift" is a signed integer value
----------------
Should this be "false if not"?

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1742
@@ +1741,3 @@
+  for (int i = 0, e = BitValues.size(); i != e; ++i) {
+    if (BitProvenance[i] == -1 || BitProvenance[i] == i) {
+      IgnoreMask.setBit(i);
----------------
I don't understand the logic here.  If `BitProvenance[i] == i` then I suppose that means the `i`th bit of the output is the `i`th bit of `V`.  How does that bit end up in the instruction you return?

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1759
@@ +1758,3 @@
+
+  // If we only have a partial match (IgnoreMask != 0), it's possible that we
+  // started looking at the interior of a larger pattern. Instead of being
----------------
Will teaching `CollectBitParts` about the `bswap` and `bitreverse` intrinsics have the same effect.  If so, that sounds slightly better.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1788
@@ -1752,2 +1787,3 @@
   Function *F = Intrinsic::getDeclaration(M, Intrin, ITy);
-  return CallInst::Create(F, V);
+  auto *CI = CallInst::Create(F, V);
+
----------------
Might want to use `Builder->CreateCall` here, so that the call instruction gets enqueued in the worklist.


Repository:
  rL LLVM

http://reviews.llvm.org/D15496