[PATCH] [InstCombine] Minor optimization for bswap with binary ops

Quentin Colombet qcolombet at apple.com
Mon Dec 1 13:02:31 PST 2014


Hi Simon,

This LGTM with a couple of minor fixes. See my inline comments.

Also, for the tests, please use opt -instnamer on the input to rename the variables. Indeed, having numbered variables makes the tests harder to update.
Moreover, I know that this tests check that we end up with no bswap. To this purpose, you added a bswap at the end of each of the computation. I believe that it complicates the test cases with no value. I think it would be best to use simpler test cases with CHECK lines, but you will have to fix all the tests.
I leave that up to you and it can be done as a subsequent commit.

Thanks,
-Quentin

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:148
@@ +147,3 @@
+  if ((IsBswapLHS || IsBswapRHS) && (IsBswapLHS || ConstLHS)
+                                 && (IsBswapRHS || ConstRHS)) {
+    Value *NewLHS = IsBswapLHS ? IntrLHS->getOperand(0) :
----------------
Use early exit.

================
Comment at: test/Transforms/InstCombine/bswap-fold.ll:70
@@ +69,3 @@
+; Fold: OP( BSWAP(x), CONSTANT ) -> BSWAP( OP(x, BSWAP(CONSTANT) ) )
+define i16 @bs_and16i(i16 %a, i16 %b) #0 {
+  %1 = tail call i16 @llvm.bswap.i16(i16 %a)
----------------
Get rid of the #0.

================
Comment at: test/Transforms/InstCombine/bswap-fold.ll:75
@@ +74,3 @@
+  ret i16 %3
+}define i16 @bs_and16(i16 %a, i16 %b) #0 {
+  %1 = tail call i16 @llvm.bswap.i16(i16 %a)
----------------
Add a new line.

http://reviews.llvm.org/D6407






More information about the llvm-commits mailing list