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

[Renato Golin via llvm-commits]

rengolin added a subscriber: rengolin.
rengolin added a comment.

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?

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


================
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())) &&
----------------
You don't seem to have added tests for the AND case.

================
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
 
----------------
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?


Repository:
  rL LLVM

http://reviews.llvm.org/D12637