Add missing barrier, fix codegen bugs, optimize codegen

dotnet · Feb 1, 2024 · 1d24a02 · 1d24a02
1 parent 2e5cdd3
commit 1d24a02
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 23 deletions.
diff --git a/src/coreclr/jit/codegenarm.cpp b/src/coreclr/jit/codegenarm.cpp
@@ -703,8 +703,8 @@ void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
     emitAttr dataSize = emitActualTypeSize(data);
 
     regNumber tempReg  = treeNode->ExtractTempReg(RBM_ALLINT);
-    regNumber loadReg  = (targetReg != REG_NA) ? targetReg : treeNode->ExtractTempReg(RBM_ALLINT);
-    regNumber storeReg = dataReg;
+    regNumber storeReg = (treeNode->OperGet() == GT_XCHG) ? dataReg : treeNode->ExtractTempReg(RBM_ALLINT);
+    regNumber loadReg  = (targetReg != REG_NA) ? targetReg : storeReg;
 
     // Check allocator assumptions
     //
@@ -743,9 +743,6 @@ void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
     //     bne retry
     //     dmb ish
 
-    BasicBlock* labelRetry = genCreateTempLabel();
-    genDefineTempLabel(labelRetry);
-
     instruction insLd = INS_ldrex;
     instruction insSt = INS_strex;
     if (varTypeIsByte(treeNode->TypeGet()))
@@ -759,12 +756,16 @@ void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
         insSt = INS_strexh;
     }
 
+    instGen_MemoryBarrier();
+
+    BasicBlock* labelRetry = genCreateTempLabel();
+    genDefineTempLabel(labelRetry);
+
     // The following instruction includes a acquire half barrier
     GetEmitter()->emitIns_R_R(insLd, dataSize, loadReg, addrReg);
 
     if (treeNode->OperGet() == GT_XADD)
     {
-        storeReg = loadReg;
         if (data->isContainedIntOrIImmed())
         {
             genInstrWithConstant(INS_add, dataSize, storeReg, loadReg, data->AsIntConCommon()->IconValue(),
@@ -864,10 +865,6 @@ void CodeGen::genCodeForCmpXchg(GenTreeCmpXchg* treeNode)
     //   compareFail:
     //     dmb ish
 
-    BasicBlock* labelRetry       = genCreateTempLabel();
-    BasicBlock* labelCompareFail = genCreateTempLabel();
-    genDefineTempLabel(labelRetry);
-
     instruction insLd = INS_ldrex;
     instruction insSt = INS_strex;
     if (varTypeIsByte(treeNode->TypeGet()))
@@ -881,20 +878,34 @@ void CodeGen::genCodeForCmpXchg(GenTreeCmpXchg* treeNode)
         insSt = INS_strexh;
     }
 
+    instGen_MemoryBarrier();
+
+    BasicBlock* labelRetry       = genCreateTempLabel();
+    BasicBlock* labelCompareFail = genCreateTempLabel();
+    genDefineTempLabel(labelRetry);
+
     // The following instruction includes a acquire half barrier
     GetEmitter()->emitIns_R_R(insLd, dataSize, targetReg, addrReg);
 
     if (comparand->isContainedIntOrIImmed())
     {
-        assert(comparand->AsIntConCommon()->IconValue() <= INT32_MAX);
-        GetEmitter()->emitIns_R_I(INS_cmp, EA_4BYTE, targetReg,
-                                  (target_ssize_t)comparand->AsIntConCommon()->IconValue());
+        if (comparand->IsIntegralConst(0))
+        {
+            GetEmitter()->emitIns_J_R(INS_cbnz, EA_4BYTE, labelCompareFail, targetReg);
+        }
+        else
+        {
+            assert(comparand->AsIntConCommon()->IconValue() <= INT32_MAX);
+            GetEmitter()->emitIns_R_I(INS_cmp, EA_4BYTE, targetReg,
+                                      (target_ssize_t)comparand->AsIntConCommon()->IconValue());
+            GetEmitter()->emitIns_J(INS_bne, labelCompareFail);
+        }
     }
     else
     {
         GetEmitter()->emitIns_R_R(INS_cmp, EA_4BYTE, targetReg, comparandReg);
+        GetEmitter()->emitIns_J(INS_bne, labelCompareFail);
     }
-    GetEmitter()->emitIns_J(INS_bne, labelCompareFail);
 
     // The following instruction includes a release half barrier
     GetEmitter()->emitIns_R_R_R(insSt, dataSize, exResultReg, dataReg, addrReg);

diff --git a/src/coreclr/jit/instrsarm.h b/src/coreclr/jit/instrsarm.h
@@ -439,7 +439,7 @@ INST1(strd,    "strd",   0,ST, IF_T2_G0,  0xE8400000)
                                            //  Rt,RT,[Rn],+-i8{!}T2_G0     1110100PU1W0nnnn ttttTTTTiiiiiiii   E840 0000
 INST1(strex,   "strex",  0,ST, IF_T2_O3,  0xE8400000)
                                            //  Rd,Rt,[Rn+i8]     T2_H1     111010000100nnnn ttttddddiiiiiiii   E840 0F00           imm(0-255)
-INST1(strexb,  "strexb", 0,ST, IF_T2_O2,  0xE8C00F50)
+INST1(strexb,  "strexb", 0,ST, IF_T2_O2,  0xE8C00F40)
                                            //  Rd,Rt,[Rn]        T2_E1     111010001100nnnn tttt11110100dddd   E8C0 0F4F
 INST1(strexd,  "strexd", 0,ST, IF_T2_O1,  0xE8C00070)
                                            //  Rd,Rt,RT,[Rn]     T2_G1     111010001100nnnn ttttTTTT0111dddd   E8C0 007F

diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp
@@ -629,13 +629,15 @@ GenTree* Lowering::LowerNode(GenTree* node)
             LowerStoreLocCommon(node->AsLclVarCommon());
             break;
 
-#if defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
+#if defined(TARGET_ARM64) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
         case GT_CMPXCHG:
             CheckImmedAndMakeContained(node, node->AsCmpXchg()->Comparand());
             break;
 
+#ifndef TARGET_ARM
         case GT_XORR:
         case GT_XAND:
+#endif // TARGET_ARM
         case GT_XADD:
             CheckImmedAndMakeContained(node, node->AsOp()->gtOp2);
             break;

diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp
@@ -82,20 +82,29 @@ bool Lowering::IsContainableImmed(GenTree* parentNode, GenTree* childNode) const
         {
             case GT_ADD:
             case GT_SUB:
-#ifdef TARGET_ARM64
+#ifdef TARGET_ARM
+            case GT_XADD:
+                return emitter::emitIns_valid_imm_for_add(immVal, flags);
+#else
                 return emitter::emitIns_valid_imm_for_add(immVal, size);
-            case GT_CMPXCHG:
+
+            case GT_XADD:
             case GT_LOCKADD:
             case GT_XORR:
             case GT_XAND:
-            case GT_XADD:
                 return comp->compOpportunisticallyDependsOn(InstructionSet_Atomics)
                            ? false
                            : emitter::emitIns_valid_imm_for_add(immVal, size);
-#elif defined(TARGET_ARM)
-                return emitter::emitIns_valid_imm_for_add(immVal, flags);
-#endif
-                break;
+#endif // TARGET_ARM
+
+            case GT_CMPXCHG:
+#ifdef TARGET_ARM
+                return emitter::emitIns_valid_imm_for_cmp(immVal, flags);
+#else
+                return comp->compOpportunisticallyDependsOn(InstructionSet_Atomics)
+                           ? false
+                           : emitter::emitIns_valid_imm_for_cmp(immVal, size);
+#endif // TARGET_ARM
 
 #ifdef TARGET_ARM64
             case GT_EQ:

diff --git a/src/coreclr/jit/lsraarm.cpp b/src/coreclr/jit/lsraarm.cpp
@@ -609,6 +609,10 @@ int LinearScan::BuildNode(GenTree* tree)
             srcCount = tree->gtGetOp2()->isContained() ? 1 : 2;
 
             buildInternalIntRegisterDefForNode(tree);
+            if (tree->OperGet() != GT_XCHG)
+            {
+                buildInternalIntRegisterDefForNode(tree);
+            }
 
             assert(!tree->gtGetOp1()->isContained());
             RefPosition* op1Use = BuildUse(tree->gtGetOp1());