[AARCH64][DAGCombine] Add combine for negation of CSEL absolute value pattern.

[sunho]

Summary

Fixes https://github.com/llvm/llvm-project/issues/51558.

The changes were ported from https://reviews.llvm.org/D111858 with some differences specific to aarch64 backend.

[sunho]

Edit typos

[sunho]

Edit existing test

[dmgreen]

Hello. This sounds good. It is similar to the patch in D112204 (but I din't have any time to continue that, glad to see you looking into the same thing!)

Feel free to steal what is useful from it, if you thing the suggested simplification is a good idea.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

14699

I think we might be able to generalize this a bit, possibly simplifying it in the process. If we have neg (csel x, y, cc), we can convert to csel (neg x), (neg y), cc, and providing at least one of x and y simplify it will be the same cost or cheaper (with a csneg operation).

I think that means that the condition is unimportant, and we won't need to emit the Add. It won't handle all the same patterns this does, both might be quite rare for cases that are not -abs (as llvm will have often optimized before this point already), but might be more straight forward.

17671

I think you can include this in performAddSubCombine, just with a check that the opcode is ISD::SUB in there or performCombineSubABS.

[sunho]

I like your method! It's always reducing negation or keeping the same number of negation unless x and y are both non-negative and it can account for nabs case very well. I was trying to tidy your patch a bit and add some more test cases to address the comments by david-arm and submit it here. However, one thing came to concern me.

When one of x and y are negative maximum of that integer type (e.g. -128 for int8), then folding negation can nullify signed integer overflows that would have happended if not folded. For instance, if you have -csel(-x,-y), then it will be folded to csel(x,y). If x was the negative maximum of that integer type, it would lead to singed integer overflow in the original form (-(-128)), but it will not after folded.

I'm not sure whether I can just consider this case UB and fold it. Could you give me some advice on this?

[sunho]

IIUC, if nsw flag is not specified it will wrap around, then -(-128) = -128, still yielding the same result. Even if nsw flag is specified, I can define poison value as anything I want so it's still fine?

[sunho]

https://alive2.llvm.org/ce/z/2KoTdr It always passes unless I put nsw on sub in tgt block. I guess that means it's okay.

[sunho]

Use simpler pattern based on https://reviews.llvm.org/D112204

[sunho]

Remove space.

[sunho]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

14730

I've added isNegatedInteger util function as suggested by david-arm in the original patch. The util function can be used in many places across multiple backends. I can submit a follow-up patch replacing to use this function if desired.

[dmgreen]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

14699

SDValues can be passed by value - they are pretty small.

14718

Could this now use: if (!isNegatedInteger(N))?

llvm/test/CodeGen/AArch64/neg-selects.ll

1 ↗

(On Diff #405438)

Can you pre-commit the tests and show just the differences here?

[sunho]

Sorry for the delay. I've separated commits that adds tests to https://reviews.llvm.org/D120214, and make this commit only show the difference between older version.

[dmgreen]

LGTM. Thanks.

[sunho]

Cheers! Can you commit on my behalf?

Author name: sunho
Author email: [e-mail]

[dmgreen]

Whoops. I put the wrong differential revision on that commit. Sorry about that. https://reviews.llvm.org/rGd6a9eec2382559aeae3bb87761afa1b6d351e9a5 should have pointed here instead.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

if (!isZeroDot(Dot))
     std::swap(Dot, A);
   if (!isZeroDot(Dot))
     return SDValue();
 
   return DAG.getNode(Dot.getOpcode(), SDLoc(N), VT, A, Dot.getOperand(1),
                      Dot.getOperand(2));
 }
 
+// Try to fold (sub Y, (csel X, -X, pl)) -> (add Y, (csel -X, X, pl)) when
+// condition came from (subs X, 0). This matches the CSEL expansion of
+// abs node lowered by lowerABS. By swapping the operands, we convert
+// abs to nabs. Note that (csel X, -X, pl) will be matched
+// to csneg by the CondSelectOp pattern.
+static SDValue performCombineSubABS(SDNode *N, SelectionDAG &DAG) {
+  SDValue N0 = N->getOperand(0);
+  SDValue N1 = N->getOperand(1);
+
+  // Is abs node has other uses, don't do the combine
+  if (N1.getOpcode() != AArch64ISD::CSEL || !N1.hasOneUse())
+    return SDValue();
+
+  ConstantSDNode *CCNode = cast<ConstantSDNode>(N1->getOperand(2));
+  AArch64CC::CondCode CC =
+      static_cast<AArch64CC::CondCode>(CCNode->getSExtValue());
+
+  if (CC != AArch64CC::PL && CC != AArch64CC::MI)
+    return SDValue();
+
+  // Condition should come from SUBS
+  SDValue Cmp = N1.getOperand(3);
+  if (Cmp.getOpcode() != AArch64ISD::SUBS || !isNullConstant(Cmp.getOperand(1)))
+    return SDValue();
+  assert(Cmp.getResNo() == 1 && "Unexpected result number");
+
+  // Get the X
+  SDValue X = Cmp.getOperand(0);
+
+  SDValue FalseOp = N1.getOperand(0);
+  SDValue TrueOp = N1.getOperand(1);
+
+  // CSEL operands should be X and NegX. Order doesn't matter.
+  auto IsNeg = [](SDValue Value, SDValue X) {
+    return Value.getOpcode() == ISD::SUB &&
+           isNullConstant(Value.getOperand(0)) && X == Value.getOperand(1);
+  };
+  if (!(IsNeg(FalseOp, X) && TrueOp == X) &&
+      !(IsNeg(TrueOp, X) && FalseOp == X))
+    return SDValue();
+
+  // Build a new CSEL with the operands swapped.
+  SDLoc DL(N);
+  MVT VT = N->getSimpleValueType(0);
+  SDValue Csel = DAG.getNode(AArch64ISD::CSEL, DL, VT, TrueOp, FalseOp,
+                             N1.getOperand(2), Cmp);
+  // Convert sub to add.
+  return DAG.getNode(ISD::ADD, DL, VT, N0, Csel);
+}
+
 // The basic add/sub long vector instructions have variants with "2" on the end
 // which act on the high-half of their inputs. They are normally matched by
 // patterns like:
 //
 // (add (zeroext (extract_high LHS)),
 //      (zeroext (extract_high RHS)))
 // -> uaddl2 vD, vN, vM
 //

 
 SDValue AArch64TargetLowering::PerformDAGCombine(SDNode *N,
                                                  DAGCombinerInfo &DCI) const {
   SelectionDAG &DAG = DCI.DAG;
   switch (N->getOpcode()) {
   default:
     LLVM_DEBUG(dbgs() << "Custom combining: skipping\n");
     break;
-  case ISD::ADD:
   case ISD::SUB:
+    if (SDValue Val = performCombineSubABS(N, DAG)) {
+      return Val;
+    }
+    LLVM_FALLTHROUGH;
+  case ISD::ADD:
     return performAddSubCombine(N, DCI, DAG);
   case ISD::XOR:
     return performXorCombine(N, DAG, DCI, Subtarget);
   case ISD::MUL:
     return performMulCombine(N, DAG, DCI, Subtarget);
   case ISD::SINT_TO_FP:
   case ISD::UINT_TO_FP:
     return performIntToFpCombine(N, DAG, Subtarget);

llvm/test/CodeGen/AArch64/neg-abs.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs \
 ; RUN:   -mtriple=aarch64-unknown-unknown < %s | FileCheck %s
 
 declare i64 @llvm.abs.i64(i64, i1 immarg)
 
 define i64 @neg_abs64(i64 %x) {
 ; CHECK-LABEL: neg_abs64:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    cmp x0, #0
-; CHECK-NEXT:    cneg x8, x0, mi
-; CHECK-NEXT:    neg x0, x8
+; CHECK-NEXT:    cneg x0, x0, pl
 ; CHECK-NEXT:    ret
   %abs = tail call i64 @llvm.abs.i64(i64 %x, i1 true)
   %neg = sub nsw i64 0, %abs
   ret i64 %neg
 }
 
 declare i32 @llvm.abs.i32(i32, i1 immarg)
 
 define i32 @neg_abs32(i32 %x) {
 ; CHECK-LABEL: neg_abs32:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    cmp w0, #0
-; CHECK-NEXT:    cneg w8, w0, mi
-; CHECK-NEXT:    neg w0, w8
+; CHECK-NEXT:    cneg w0, w0, pl
 ; CHECK-NEXT:    ret
   %abs = tail call i32 @llvm.abs.i32(i32 %x, i1 true)
   %neg = sub nsw i32 0, %abs
   ret i32 %neg
 }
 
 declare i16 @llvm.abs.i16(i16, i1 immarg)