[PATCH] D15496: [InstCombine] Identify partial bitreverses

[James Molloy via llvm-commits]

jmolloy marked an inline comment as done.
jmolloy added a comment.

Hi Sanjoy,

Thanks for the review! New version coming up.

James


================
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
----------------
sanjoy wrote:
> Should this be "false if not"?
I honestly don't know. The problem is that would be a double negative ("if not unsuccessful").

The best thing to do here is for me to rewrite this function to be sane and return true on success and false on failure. I'll do this in a dedicated followup.

================
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);
----------------
sanjoy wrote:
> 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?
In a partial bitreverse/bswap, some of the bits are going to be the same as the original value. We're essentially saying that the result is a OR-ed mask of a reversed value and the original value:

   uint8_t a;
   uint8_t b = bitreverse(a);
   uint8_t c = a & M | b & ~M

This can come from something like:

  uint8_t dest = src; // In a pure bitreverse, dest would be initialized to zero.
  if (src & 1<<0) dest |= 1<<7;
  if (src & 1<<1) dest |= 1<<6;
  ...


================
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
----------------
sanjoy wrote:
> Will teaching `CollectBitParts` about the `bswap` and `bitreverse` intrinsics have the same effect.  If so, that sounds slightly better.
Indeed, that sounds much better. My new patch does this.


Repository:
  rL LLVM

http://reviews.llvm.org/D15496