[AArch64][Atomic] Canonicalize sub of immediate to add of -immediate.

[mcrosier]

Summary

Please let me know if I've missed something or if there's an easier way to do this in tablegen.

This avoids ugliness such as:

test_atomic_sub_fetch:                  // @test_atomic_sub_fetch
// BB#0:                                // %entry
        mov     w8, #-1
        neg      w8, w8
        ldaddal w8, w8, [x0]
        add     w0, w8, #1              // =1
        ret

and instead generates:

test_atomic_sub_fetch:                  // @test_atomic_sub_fetch
// BB#0:                                // %entry
        orr     w8, wzr, #0x1
        ldaddal w8, w8, [x0]
        add     w0, w8, #1              // =1
        ret

Chad

[gberry]

Perhaps this would be better done at the IR level, in InstCombine?

[mcrosier]

In D35375#808725, @gberry wrote:

Perhaps this would be better done at the IR level, in InstCombine?

Seems reasonable, but we'd have to know this is a good canonicalization for all targets, right?

[gberry]

In D35375#808771, @mcrosier wrote:

In D35375#808725, @gberry wrote:

Perhaps this would be better done at the IR level, in InstCombine?

Seems reasonable, but we'd have to know this is a good canonicalization for all targets, right?

It seems like it is to me. Worst case (e.g. the target supports 'add' but not 'sub'), the sub would ideally just be transformed back and a negate added during ISel legalization.

[christof]

In D35375#808895, @gberry wrote:

In D35375#808771, @mcrosier wrote:

In D35375#808725, @gberry wrote:

Perhaps this would be better done at the IR level, in InstCombine?

Seems reasonable, but we'd have to know this is a good canonicalization for all targets, right?

It seems like it is to me. Worst case (e.g. the target supports 'add' but not 'sub'), the sub would ideally just be transformed back and a negate added during ISel legalization.

Surely it depends on the immediate range and add/sub which are supported by the target whether or not it is good for that target? Negative immediate might be harder for some targets, I would expect. I won't be surprised if some code generators give a regression if that normalisation is done in general. They might generate worse code if they see add -1 instead of sub 1. Just my two cents.

I am not really well known with all the tricks you can do with tablegen, but I would think this particular rewrite could be done with a pattern match in tablegen, could it not? I think there is already something for add -imm to sub imm.

[gberry]

I suppose the analogous InstCombine canonicalization in this case is to convert all 'sub's of immediates to 'add's of negated immediates doesn't really apply, since there isn't much additional optimization that can be done on the atomic operations themselves.

Is there any reason to restrict this to cases where the sub operand is a constant? Since we're always going to add a sub 0, x in ISel, adding it a bit earlier in the DAG combine phase would allow for additional optimizations in the non-constant case as well (e.g. folding a double negate away).

[mcrosier]

I'm not going to pursue this further, but if someone wants to pick up this work I'd be more than happy to assist.

lib/Target/AArch64/AArch64ISelLowering.cpp

AArch64TargetLowering::AArch64TargetLowering(const TargetMachine &TM,
   setTargetDAGCombine(ISD::ZERO_EXTEND);
   setTargetDAGCombine(ISD::SIGN_EXTEND);
   setTargetDAGCombine(ISD::BITCAST);
   setTargetDAGCombine(ISD::CONCAT_VECTORS);
   setTargetDAGCombine(ISD::STORE);
   if (Subtarget->supportsAddressTopByteIgnored())
     setTargetDAGCombine(ISD::LOAD);
 
+  setTargetDAGCombine(ISD::ATOMIC_LOAD_SUB);
   setTargetDAGCombine(ISD::MUL);
 
   setTargetDAGCombine(ISD::SELECT);
   setTargetDAGCombine(ISD::VSELECT);
 
   setTargetDAGCombine(ISD::INTRINSIC_VOID);
   setTargetDAGCombine(ISD::INTRINSIC_W_CHAIN);
   setTargetDAGCombine(ISD::INSERT_VECTOR_ELT);

 /// Get rid of unnecessary NVCASTs (that don't change the type).
 static SDValue performNVCASTCombine(SDNode *N) {
   if (N->getValueType(0) == N->getOperand(0).getValueType())
     return N->getOperand(0);
 
   return SDValue();
 }
 
+/// Try to convert subtractions of constants to additions.
+static SDValue performAtomicLoadSubCombine(SDNode *N, SelectionDAG &DAG) {
+  if (!isa<ConstantSDNode>(N->getOperand(2)))
+    return SDValue();
+
+  SDLoc dl(N);
+  MVT VT = N->getSimpleValueType(0);
+  SDValue RHS = N->getOperand(2);
+  AtomicSDNode *AN = cast<AtomicSDNode>(N);
+  RHS = DAG.getNode(ISD::SUB, dl, VT, DAG.getConstant(0, dl, VT), RHS);
+  return DAG.getAtomic(ISD::ATOMIC_LOAD_ADD, dl, VT, N->getOperand(0),
+                       N->getOperand(1), RHS, AN->getMemOperand());
+}
+
 SDValue AArch64TargetLowering::PerformDAGCombine(SDNode *N,
                                                  DAGCombinerInfo &DCI) const {
   SelectionDAG &DAG = DCI.DAG;
   switch (N->getOpcode()) {
   default:
     break;
   case ISD::ADD:
   case ISD::SUB:
SDValue AArch64TargetLowering::PerformDAGCombine(SDNode *N,
   case ISD::SELECT:
     return performSelectCombine(N, DCI);
   case ISD::VSELECT:
     return performVSelectCombine(N, DCI.DAG);
   case ISD::LOAD:
     if (performTBISimplification(N->getOperand(1), DCI, DAG))
       return SDValue(N, 0);
     break;
+  case ISD::ATOMIC_LOAD_SUB:
+    return performAtomicLoadSubCombine(N, DAG);
   case ISD::STORE:
     return performSTORECombine(N, DCI, DAG, Subtarget);
   case AArch64ISD::BRCOND:
     return performBRCONDCombine(N, DCI, DAG);
   case AArch64ISD::TBNZ:
   case AArch64ISD::TBZ:
     return performTBZCombine(N, DCI, DAG);
   case AArch64ISD::CSEL:

test/CodeGen/AArch64/atomic-ops-lse.ll

 ; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var64
 
 ; CHECK: ldaddal x[[NEG]], x[[NEW:[0-9]+]], [x[[ADDR]]]
 ; CHECK-NOT: dmb
 
   ret void
 }
 
+define i8 @test_atomic_load_sub_i8_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i8_neg_imm:
+  %old = atomicrmw sub i8* @var8, i8 -1 seq_cst
+
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var8
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var8
+; CHECK: orr w[[IMM:[0-9]+]], wzr, #0x1
+; CHECK: ldaddalb w[[IMM]], w[[NEW:[0-9]+]], [x[[ADDR]]]
+; CHECK-NOT: dmb
+
+  ret i8 %old
+}
+
+define i16 @test_atomic_load_sub_i16_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i16_neg_imm:
+  %old = atomicrmw sub i16* @var16, i16 -1 seq_cst
+
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var16
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var16
+; CHECK: orr w[[IMM:[0-9]+]], wzr, #0x1
+; CHECK: ldaddalh w[[IMM]], w[[NEW:[0-9]+]], [x[[ADDR]]]
+; CHECK-NOT: dmb
+
+  ret i16 %old
+}
+
+define i32 @test_atomic_load_sub_i32_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i32_neg_imm:
+  %old = atomicrmw sub i32* @var32, i32 -1 seq_cst
+
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var32
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var32
+; CHECK: orr w[[IMM:[0-9]+]], wzr, #0x1
+; CHECK: ldaddal w[[IMM]], w[[NEW:[0-9]+]], [x[[ADDR]]]
+; CHECK-NOT: dmb
+
+  ret i32 %old
+}
+
+define i64 @test_atomic_load_sub_i64_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i64_neg_imm:
+  %old = atomicrmw sub i64* @var64, i64 -1 seq_cst
+
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var64
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var64
+; CHECK: orr w[[IMM:[0-9]+]], wzr, #0x1
+; CHECK: ldaddal x[[IMM]], x[[NEW:[0-9]+]], [x[[ADDR]]]
+; CHECK-NOT: dmb
+
+  ret i64 %old
+}
+
 define i8 @test_atomic_load_and_i8(i8 %offset) nounwind {
 ; CHECK-LABEL: test_atomic_load_and_i8:
   %old = atomicrmw and i8* @var8, i8 %offset seq_cst
 ; CHECK-NOT: dmb
 ; CHECK: mvn w[[NOT:[0-9]+]], w[[OLD:[0-9]+]]
 ; CHECK: adrp [[TMPADDR:x[0-9]+]], var8
 ; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var8
 

test/CodeGen/AArch64/atomic-ops.ll

 ; CHECK-NEXT: stlxr [[STATUS:w[0-9]+]], [[NEW]], [x[[ADDR]]]
 ; CHECK-NEXT: cbnz [[STATUS]], .LBB{{[0-9]+}}_1
 ; CHECK-NOT: dmb
 
 ; CHECK: mov x0, x[[OLD]]
    ret i64 %old
 }
 
+define i8 @test_atomic_load_sub_i8_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i8_neg_imm:
+   %old = atomicrmw sub i8* @var8, i8 -1 monotonic
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var8
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var8
+
+; CHECK: .LBB{{[0-9]+}}_1:
+; CHECK: ldxrb w[[OLD:[0-9]+]], [x[[ADDR]]]
+; CHECK-NEXT: add [[NEW:w[0-9]+]], w[[OLD]], #1
+; CHECK-NEXT: stxrb [[STATUS:w[0-9]+]], [[NEW]], [x[[ADDR]]]
+; CHECK-NEXT: cbnz [[STATUS]], .LBB{{[0-9]+}}_1
+; CHECK-NOT: dmb
+
+   ret i8 %old
+}
+
+define i16 @test_atomic_load_sub_i16_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i16_neg_imm:
+   %old = atomicrmw sub i16* @var16, i16 -1 release
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var16
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var16
+
+; CHECK: .LBB{{[0-9]+}}_1:
+; CHECK: ldxrh w[[OLD:[0-9]+]], [x[[ADDR]]]
+; CHECK-NEXT: add [[NEW:w[0-9]+]], w[[OLD]], #1
+; CHECK-NEXT: stlxrh [[STATUS:w[0-9]+]], [[NEW]], [x[[ADDR]]]
+; CHECK-NEXT: cbnz [[STATUS]], .LBB{{[0-9]+}}_1
+; CHECK-NOT: dmb
+
+   ret i16 %old
+}
+
+define i32 @test_atomic_load_sub_i32_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i32_neg_imm:
+   %old = atomicrmw sub i32* @var32, i32 -1 acquire
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var32
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var32
+
+; CHECK: .LBB{{[0-9]+}}_1:
+; CHECK: ldaxr w[[OLD:[0-9]+]], [x[[ADDR]]]
+; CHECK-NEXT: add [[NEW:w[0-9]+]], w[[OLD]], #1
+; CHECK-NEXT: stxr [[STATUS:w[0-9]+]], [[NEW]], [x[[ADDR]]]
+; CHECK-NEXT: cbnz [[STATUS]], .LBB{{[0-9]+}}_1
+; CHECK-NOT: dmb
+
+   ret i32 %old
+}
+
+define i64 @test_atomic_load_sub_i64_neg_imm() nounwind {
+; CHECK-LABEL: test_atomic_load_sub_i64_neg_imm:
+   %old = atomicrmw sub i64* @var64, i64 -1 seq_cst
+; CHECK-NOT: dmb
+; CHECK: adrp [[TMPADDR:x[0-9]+]], var64
+; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var64
+
+; CHECK: .LBB{{[0-9]+}}_1:
+; CHECK: ldaxr x[[OLD:[0-9]+]], [x[[ADDR]]]
+; CHECK-NEXT: add [[NEW:x[0-9]+]], x[[OLD]], #1
+; CHECK-NEXT: stlxr [[STATUS:w[0-9]+]], [[NEW]], [x[[ADDR]]]
+; CHECK-NEXT: cbnz [[STATUS]], .LBB{{[0-9]+}}_1
+; CHECK-NOT: dmb
+
+   ret i64 %old
+}
+
 define i8 @test_atomic_load_and_i8(i8 %offset) nounwind {
 ; CHECK-LABEL: test_atomic_load_and_i8:
    %old = atomicrmw and i8* @var8, i8 %offset release
 ; CHECK-NOT: dmb
 ; CHECK: adrp [[TMPADDR:x[0-9]+]], var8
 ; CHECK: add x[[ADDR:[0-9]+]], [[TMPADDR]], {{#?}}:lo12:var8
 
 ; CHECK: .LBB{{[0-9]+}}_1: