[PATCH] D113948: [DAGCombiner][X86] Prevent unfoldMaskedMerge from creating an AND with two inverted inputs.

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

Regarding the comment about X: yes, that's wanted. Here's the test:

  define i32 @foo(i32 %y, i32 %mask) {  
    %notmask = xor i32 %mask, -1
    %n0 = xor i32 42, %y
    %n1 = and i32 %n0, %notmask
    %r = xor i32 %n1, %y
    ret i32 %r
  }

which (still) gives:

  movl    %esi, %eax
  andl    %esi, %edi
  notl    %eax
  andl    $42, %eax
  orl     %edi, %eax

but with the below patch (didn't fully deal with the comments before the if, though I did update the function header comment which is currently out of sync in this patch):

  --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  @@ -7784,9 +7784,12 @@ SDValue DAGCombiner::MatchLoadCombine(SDNode *N) {
   //    |  D  |
   // Into:
   //   (x & m) | (y & ~m)
  -// If y is a constant, and the 'andn' does not work with immediates,
  -// we unfold into a different pattern:
  +// If y is a constant, m is not a 'not', and the 'andn' does not work with
  +// immediates, we unfold into a different pattern:
   //   ~(~x & m) & (m | y)
  +// If x is a constant, m is a 'not', and the 'andn' does not work with
  +// immediates, we unfold into a different pattern:
  +//   (x | ~m) & ~(~m & ~y)
   // NOTE: we don't unfold the pattern if 'xor' is actually a 'not', because at
   //       the very least that breaks andnpd / andnps patterns, and because those
   //       patterns are simplified in IR and shouldn't be created in the DAG
  @@ -7842,7 +7845,7 @@ SDValue DAGCombiner::unfoldMaskedMerge(SDNode *N) {
     SDLoc DL(N);
   
     // If Y is a constant, check that 'andn' works with immediates.
  -  if (!TLI.hasAndNot(Y)) {
  +  if (!TLI.hasAndNot(Y) && !isBitwiseNot(M)) {
       assert(TLI.hasAndNot(X) && "Only mask is a variable? Unreachable.");
       // If not, we need to do a bit more work to make sure andn is still used.
       SDValue NotX = DAG.getNOT(DL, X, VT);
  @@ -7852,6 +7855,17 @@ SDValue DAGCombiner::unfoldMaskedMerge(SDNode *N) {
       return DAG.getNode(ISD::AND, DL, VT, NotLHS, RHS);
     }
   
  +  if (!TLI.hasAndNot(X) && isBitwiseNot(M)) {
  +    assert(TLI.hasAndNot(Y) && "Only mask is a variable? Unreachable.");
  +    // If not, we need to do a bit more work to make sure andn is still used.
  +    SDValue NotM = DAG.getNOT(DL, M, VT);
  +    SDValue LHS = DAG.getNode(ISD::OR, DL, VT, X, NotM);
  +    SDValue NotY = DAG.getNOT(DL, Y, VT);
  +    SDValue RHS = DAG.getNode(ISD::AND, DL, VT, NotM, NotY);
  +    SDValue NotRHS = DAG.getNOT(DL, RHS, VT);
  +    return DAG.getNode(ISD::AND, DL, VT, LHS, NotRHS);
  +  }
  +
     SDValue LHS = DAG.getNode(ISD::AND, DL, VT, X, M);
     SDValue NotM = DAG.getNOT(DL, M, VT);
     SDValue RHS = DAG.getNode(ISD::AND, DL, VT, Y, NotM);

(which I think is correct...) gives:

  andnl   %esi, %edi, %eax
  orl     $42, %esi
  andnl   %esi, %eax, %eax

No other file in llvm/test/CodeGen/X86 seems to be affected, though I haven't checked other targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113948