[PATCH] D12637: [InstCombine] Recognize another bswap idiom.

[Charlie Turner via llvm-commits]

chatur01 added a comment.

Hi Renato! Thanks for looking at my patch.

> I'm assuming that the bswap builtin will be triggered in different ways for different target, but that's not what the test here is trying to do, right?


I thought my transformation was target-independent at this level.

> Are there tests to make sure that this built-in expands to the correct instructions in the different types of ARM architectures? If not, I'd add that, too.


There's certainly some, I don't think they're exhaustive of all possibilities. I'll add the test I have to ARM tests at least, but I'll have to do a more intensive sweep of the current tests to make sure they're covering all the possibilities. Sorry for the oversight, this review will have to stall until I've completed that task.

Thanks again Renato.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2211
@@ +2210,3 @@
+                    match(Op1, m_LogicalShift(m_Value(), m_Value()));
+  // (A & B) | (C & D)                              -> bswap if possible.
+  bool OrOfAnds = match(Op0, m_And(m_Value(), m_Value())) &&
----------------
rengolin wrote:
> You don't seem to have added tests for the AND case.
The test I added checks for the OrOfAnds:
%or6 = or i32 %shl3, %shr5
Where the misnamed "shl3" and "shr5" are and instructions.

================
Comment at: test/Transforms/InstCombine/bswap.ll:4
@@ -3,3 +3,3 @@
 ; RUN: opt < %s -instcombine -S | \
-; RUN:    grep "call.*llvm.bswap" | count 6
+; RUN:    grep "call.*llvm.bswap" | count 7
 
----------------
rengolin wrote:
> Not related to this patch, but we stopped using grep a long time ago...
> 
> Maybe it'd be simple enough to add the CHECK lines?
Please can such a change come in a different commit?


Repository:
  rL LLVM

http://reviews.llvm.org/D12637