[PATCH] D149699: [InstCombine] Improve bswap optimization

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
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;
----------------
austin880625 wrote:
> goldstein.w.n wrote:
> > What is this change for?
> if one of the operand has more than one use, then the transform will not reduce the number of instructions(can look at the `bs_and64_multiuse*` test case diff) and in DAGCombiner the same optimization also uses `||` instead of `&&`:
> 
> ```
>   // Unary ops: logic_op (bswap x), (bswap y) --> bswap (logic_op x, y)
>   if (HandOpcode == ISD::BSWAP) {
>     // If either operand has other uses, this transform is not an improvement.
>     if (!N0.hasOneUse() || !N1.hasOneUse())
>       return SDValue();
>     SDValue Logic = DAG.getNode(LogicOpcode, DL, XVT, X, Y);
>     return DAG.getNode(HandOpcode, DL, VT, Logic);                                   
>   }
> ```
> 
> I noticed it just because I was referencing D6407 , It's minor and not directly related to the improvement this time. I'm not sure if this kind of change can be included.
> if one of the operand has more than one use, then the transform will not reduce the number of instructions(can look at the `bs_and64_multiuse*` test case diff) and in DAGCombiner the same optimization also uses `||` instead of `&&`:
> 
> ```
>   // Unary ops: logic_op (bswap x), (bswap y) --> bswap (logic_op x, y)
>   if (HandOpcode == ISD::BSWAP) {
>     // If either operand has other uses, this transform is not an improvement.
>     if (!N0.hasOneUse() || !N1.hasOneUse())
>       return SDValue();
>     SDValue Logic = DAG.getNode(LogicOpcode, DL, XVT, X, Y);
>     return DAG.getNode(HandOpcode, DL, VT, Logic);                                   
>   }
> ```
> 
> I noticed it just because I was referencing D6407 , It's minor and not directly related to the improvement this time. I'm not sure if this kind of change can be included.

It should be a seperate patch.

Its generally okay for a combine to result in the same amount of instructions so if you have a case where this is useful its fine to post as another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149699