[PATCH] D149699: [InstCombine] Improve bswap optimization

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6029
     // bswap(bswap(x)) -> x
+    // The duplicate bswap/bitreverse might not come from real world code, but
+    // can be introduced by other optimizations
----------------
"bswap/bitreverse" -> "bswap"


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10756
 
+  // fold (bswap (logic_op(bswap(x),y))) -> logic_op(x,bswap(y))
+  // Need to ensure logic_op and bswap(x) doesn't have other uses
----------------
Likewise a helper here so it can be easily extended to handle bitreverse/any other intrinsics that it applies to.
Also, there are no tests for the DAGCombiner changes at the moment.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:70
     // OP( BSWAP(x), BSWAP(y) ) -> BSWAP( OP(x, y) )
-    if (!OldLHS->hasOneUse() && !OldRHS->hasOneUse())
+    if (!OldLHS->hasOneUse() || !OldRHS->hasOneUse())
       return nullptr;
----------------
What is this change for?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1275
+template <Intrinsic::ID IntrID>
+Instruction *InstCombinerImpl::foldBitOrderCrossLogicOp(Value *V) {
+  if (IntrID != Intrinsic::bswap && IntrID != Intrinsic::bitreverse)
----------------
Instead of making this a member function. Can you leave it `static`. To create new instructions you can add an extra argument `InstCombiner::BuilderTy &Builder`.

It should look like: `InstCombineAndOrXor.cc::reassociateFCmps`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1708
+      return crossLogicOpFold;
+    }
+
----------------
```
 if (Instruction *crossLogicOpFold = foldBitOrderCrossLogicOp<Intrinsic::bswap>(IIOperand))
    return crossLogicOpFold;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149699/new/

https://reviews.llvm.org/D149699