Skip to content

Commit c993ba2

Browse files
jakobbotschjakobbotsch
authored
JIT: Refine asserts for RMW check in crc32 and hw intrinsic codegen (#108750)
The crc32 HW intrinsic is a 2 operand node that produces a register; however, the underlying instruction is an RMW instruction that destructively modifies `op1`. For that reason the codegen needs to issue a move of op1 into the target register before `crc32` is issued. There is an assert that this `mov` does not overwrite op2 in the process. When LSRA builds uses for the operands it needs to mark op2 as delay freed to ensure op2 and the target do not end up in the same register. However, due to optimizations LSRA may skip this marking when op1 is a last use. Thus we can end up hitting the assert. The most logical solution seems to be to actually ensure that op2 is always delay freed, such that it never shares the register with the target. However, the handling for other RMW nodes (like `GT_SUB`) does not have similar logic to this; instead it relies on the situation to not happen except for one particular case where it is ok: when `op1` and `op2` are actually the same local. This PR enhances the assert to match the checks that happen for other RMW nodes. Also enhance similar asserts for arm64 HW intrinsics.
1 parent f868efe commit c993ba2

File tree

5 files changed

+59
-44
lines changed

5 files changed

+59
-44
lines changed

‎src/coreclr/jit/codegen.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,7 @@ class CodeGen final : public CodeGenInterface
742742
#endif
743743
void genCodeForTreeNode(GenTree* treeNode);
744744
void genCodeForBinary(GenTreeOp* treeNode);
745+
bool genIsSameLocalVar(GenTree* tree1, GenTree* tree2);
745746

746747
#if defined(TARGET_X86)
747748
void genCodeForLongUMod(GenTreeOp* node);

‎src/coreclr/jit/codegencommon.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,25 @@ regMaskTP CodeGenInterface::genGetRegMask(GenTree* tree)
636636
return regMask;
637637
}
638638

639+
//------------------------------------------------------------------------
640+
// genIsSameLocalVar:
641+
// Check if two trees represent the same scalar local value.
642+
//
643+
// Arguments:
644+
// op1 - first tree
645+
// op2 - second tree
646+
//
647+
// Returns:
648+
// True if so.
649+
//
650+
bool CodeGen::genIsSameLocalVar(GenTree* op1, GenTree* op2)
651+
{
652+
GenTree* op1Skip = op1->gtSkipReloadOrCopy();
653+
GenTree* op2Skip = op2->gtSkipReloadOrCopy();
654+
return op1Skip->OperIs(GT_LCL_VAR) && op2Skip->OperIs(GT_LCL_VAR) &&
655+
(op1Skip->AsLclVar()->GetLclNum() == op2Skip->AsLclVar()->GetLclNum());
656+
}
657+
639658
// The given lclVar is either going live (being born) or dying.
640659
// It might be both going live and dying (that is, it is a dead store) under MinOpts.
641660
// Update regSet.GetMaskVars() accordingly.

‎src/coreclr/jit/codegenxarch.cpp‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,28 +1143,11 @@ void CodeGen::genCodeForBinary(GenTreeOp* treeNode)
11431143
// In order for this operation to be correct
11441144
// we need that op is a commutative operation so
11451145
// we can convert it into reg1 = reg1 op reg2 and emit
1146-
// the same code as above
1146+
// the same code as above. Or we need both operands to
1147+
// be the same local.
11471148
else if (op2reg == targetReg)
11481149
{
1149-
1150-
#ifdef DEBUG
1151-
unsigned lclNum1 = (unsigned)-1;
1152-
unsigned lclNum2 = (unsigned)-2;
1153-
1154-
GenTree* op1Skip = op1->gtSkipReloadOrCopy();
1155-
GenTree* op2Skip = op2->gtSkipReloadOrCopy();
1156-
1157-
if (op1Skip->OperIsLocalRead())
1158-
{
1159-
lclNum1 = op1Skip->AsLclVarCommon()->GetLclNum();
1160-
}
1161-
if (op2Skip->OperIsLocalRead())
1162-
{
1163-
lclNum2 = op2Skip->AsLclVarCommon()->GetLclNum();
1164-
}
1165-
1166-
assert(GenTree::OperIsCommutative(oper) || (lclNum1 == lclNum2));
1167-
#endif
1150+
assert(GenTree::OperIsCommutative(oper) || genIsSameLocalVar(op1, op2));
11681151

11691152
dst = op2;
11701153
src = op1;

‎src/coreclr/jit/hwintrinsiccodegenarm64.cpp‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -368,8 +368,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
368368
{
369369
if (isRMW)
370370
{
371-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
372-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
371+
assert((targetReg == op1Reg) || (targetReg != op2Reg) || genIsSameLocalVar(intrin.op1, intrin.op2));
372+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
373373
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
374374

375375
HWIntrinsicImmOpHelper helper(this, intrin.op4, node);
@@ -412,8 +412,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
412412
{
413413
if (isRMW)
414414
{
415-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
416-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
415+
assert((targetReg == op1Reg) || (targetReg != op2Reg) || genIsSameLocalVar(intrin.op1, intrin.op2));
416+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
417417
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
418418
GetEmitter()->emitIns_R_R_R_I(ins, emitSize, targetReg, op2Reg, op3Reg, 0, opt);
419419
}
@@ -511,7 +511,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
511511
// If `falseReg` is zero, then move the first operand of `intrinEmbMask` in the
512512
// destination using /Z.
513513

514-
assert((targetReg != embMaskOp2Reg) || (embMaskOp1Reg == embMaskOp2Reg));
514+
assert((targetReg != embMaskOp2Reg) || (embMaskOp1Reg == embMaskOp2Reg) ||
515+
genIsSameLocalVar(intrinEmbMask.op1, intrinEmbMask.op2));
515516
assert(intrin.op3->isContained() || !intrin.op1->IsTrueMask(node->GetSimdBaseType()));
516517
GetEmitter()->emitInsSve_R_R_R(INS_sve_movprfx, emitSize, targetReg, maskReg, embMaskOp1Reg, opt);
517518
}
@@ -765,14 +766,16 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
765766
switch (intrinEmbMask.id)
766767
{
767768
case NI_Sve_CreateBreakPropagateMask:
768-
assert((targetReg == embMaskOp2Reg) || (targetReg != embMaskOp1Reg));
769+
assert((targetReg == embMaskOp2Reg) || (targetReg != embMaskOp1Reg) ||
770+
genIsSameLocalVar(intrinEmbMask.op1, intrinEmbMask.op2));
769771
GetEmitter()->emitIns_Mov(INS_sve_mov, emitSize, targetReg, embMaskOp2Reg,
770772
/* canSkip */ true);
771773
emitInsHelper(targetReg, maskReg, embMaskOp1Reg);
772774
break;
773775

774776
case NI_Sve_AddSequentialAcross:
775-
assert((targetReg == op1Reg) || (targetReg != embMaskOp2Reg));
777+
assert((targetReg == op1Reg) || (targetReg != embMaskOp2Reg) ||
778+
genIsSameLocalVar(intrinEmbMask.op1, intrinEmbMask.op2));
776779
GetEmitter()->emitIns_Mov(INS_fmov, GetEmitter()->optGetSveElemsize(embOpt), targetReg,
777780
embMaskOp1Reg, /* canSkip */ true);
778781
emitInsHelper(targetReg, maskReg, embMaskOp2Reg);
@@ -1061,7 +1064,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
10611064
{
10621065
if (isRMW)
10631066
{
1064-
assert((targetReg == op2Reg) || (targetReg != op1Reg));
1067+
assert((targetReg == op2Reg) || (targetReg != op1Reg) ||
1068+
genIsSameLocalVar(intrin.op1, intrin.op2));
10651069
GetEmitter()->emitIns_Mov(ins_Move_Extend(intrin.op2->TypeGet(), false),
10661070
emitTypeSize(node), targetReg, op2Reg,
10671071
/* canSkip */ true);
@@ -1081,7 +1085,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
10811085
}
10821086
else if (isRMW)
10831087
{
1084-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
1088+
assert((targetReg == op1Reg) || (targetReg != op2Reg) ||
1089+
genIsSameLocalVar(intrin.op1, intrin.op2));
10851090
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg,
10861091
/* canSkip */ true);
10871092
GetEmitter()->emitIns_R_R(ins, emitSize, targetReg, op2Reg, opt);
@@ -1099,15 +1104,21 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
10991104
{
11001105
if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id))
11011106
{
1102-
assert((targetReg == op2Reg) || ((targetReg != op1Reg) && (targetReg != op3Reg)));
1107+
assert((targetReg == op2Reg) || (targetReg != op1Reg) ||
1108+
genIsSameLocalVar(intrin.op2, intrin.op1));
1109+
assert((targetReg == op2Reg) || (targetReg != op3Reg) ||
1110+
genIsSameLocalVar(intrin.op2, intrin.op3));
11031111

11041112
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op2Reg,
11051113
/* canSkip */ true);
11061114
GetEmitter()->emitIns_R_R_R(ins, emitSize, targetReg, op1Reg, op3Reg, opt);
11071115
}
11081116
else
11091117
{
1110-
assert((targetReg == op1Reg) || ((targetReg != op2Reg) && (targetReg != op3Reg)));
1118+
assert((targetReg == op1Reg) || (targetReg != op2Reg) ||
1119+
genIsSameLocalVar(intrin.op1, intrin.op2));
1120+
assert((targetReg == op1Reg) || (targetReg != op3Reg) ||
1121+
genIsSameLocalVar(intrin.op1, intrin.op3));
11111122

11121123
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg,
11131124
/* canSkip */ true);
@@ -1358,7 +1369,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
13581369
case NI_AdvSimd_InsertScalar:
13591370
{
13601371
assert(isRMW);
1361-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
1372+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
13621373
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
13631374

13641375
HWIntrinsicImmOpHelper helper(this, intrin.op2, node);
@@ -1375,7 +1386,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
13751386
case NI_AdvSimd_Arm64_InsertSelectedScalar:
13761387
{
13771388
assert(isRMW);
1378-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
1389+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
13791390
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
13801391

13811392
const int resultIndex = (int)intrin.op2->AsIntCon()->gtIconVal;
@@ -1387,7 +1398,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
13871398
case NI_AdvSimd_LoadAndInsertScalar:
13881399
{
13891400
assert(isRMW);
1390-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
1401+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
13911402
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
13921403

13931404
HWIntrinsicImmOpHelper helper(this, intrin.op2, node);
@@ -1983,7 +1994,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
19831994
break;
19841995
}
19851996

1986-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
1997+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
19871998
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
19881999
GetEmitter()->emitIns_R_R_R(ins, emitSize, targetReg, op2Reg, op3Reg, opt);
19892000
break;
@@ -2348,8 +2359,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
23482359
case NI_Sve_SaturatingIncrementBy8BitElementCount:
23492360
{
23502361
assert(isRMW);
2351-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
2352-
assert((targetReg == op1Reg) || (targetReg != op3Reg));
2362+
assert((targetReg == op1Reg) || (targetReg != op2Reg) || genIsSameLocalVar(intrin.op1, intrin.op2));
2363+
assert((targetReg == op1Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op1, intrin.op3));
23532364
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
23542365

23552366
if (intrin.op2->IsCnsIntOrI() && intrin.op3->IsCnsIntOrI())
@@ -2402,7 +2413,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
24022413
case NI_Sve_SaturatingIncrementByActiveElementCount:
24032414
{
24042415
// RMW semantics
2405-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
2416+
assert((targetReg == op1Reg) || (targetReg != op2Reg) || genIsSameLocalVar(intrin.op1, intrin.op2));
24062417
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
24072418

24082419
// Switch instruction if arg1 is unsigned.
@@ -2442,7 +2453,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
24422453
case NI_Sve_ExtractVector:
24432454
{
24442455
assert(isRMW);
2445-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
2456+
assert((targetReg == op1Reg) || (targetReg != op2Reg) || genIsSameLocalVar(intrin.op1, intrin.op2));
24462457
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true);
24472458

24482459
HWIntrinsicImmOpHelper helper(this, intrin.op3, node);
@@ -2461,7 +2472,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
24612472
{
24622473
assert(isRMW);
24632474
assert(emitter::isFloatReg(op2Reg) == varTypeIsFloating(intrin.baseType));
2464-
assert((targetReg == op1Reg) || (targetReg != op2Reg));
2475+
assert((targetReg == op1Reg) || (targetReg != op2Reg) || genIsSameLocalVar(intrin.op1, intrin.op2));
24652476
GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg,
24662477
/* canSkip */ true);
24672478
GetEmitter()->emitInsSve_R_R(ins, emitSize, targetReg, op2Reg, opt);
@@ -2487,7 +2498,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
24872498
{
24882499
assert(isRMW);
24892500
assert(HWIntrinsicInfo::IsExplicitMaskedOperation(intrin.id));
2490-
assert((targetReg == op2Reg) || (targetReg != op1Reg));
2501+
assert((targetReg == op2Reg) || (targetReg != op1Reg) || genIsSameLocalVar(intrin.op2, intrin.op1));
24912502
GetEmitter()->emitIns_Mov(INS_sve_mov, emitTypeSize(node), targetReg, op2Reg, /* canSkip */ true);
24922503
GetEmitter()->emitIns_R_R(ins, emitSize, targetReg, op1Reg, INS_OPTS_SCALABLE_B);
24932504
break;
@@ -2559,8 +2570,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
25592570
{
25602571
assert(emitter::isFloatReg(targetReg));
25612572
assert(varTypeIsFloating(node->gtType) || varTypeIsSIMD(node->gtType));
2562-
assert((targetReg == op2Reg) || (targetReg != op1Reg));
2563-
assert((targetReg == op2Reg) || (targetReg != op3Reg));
2573+
assert((targetReg == op2Reg) || (targetReg != op1Reg) || genIsSameLocalVar(intrin.op2, intrin.op1));
2574+
assert((targetReg == op2Reg) || (targetReg != op3Reg) || genIsSameLocalVar(intrin.op2, intrin.op3));
25642575

25652576
GetEmitter()->emitIns_Mov(INS_sve_mov, EA_SCALABLE, targetReg, op2Reg, /* canSkip */ true, opt);
25662577
GetEmitter()->emitInsSve_R_R_R(ins, EA_SCALABLE, targetReg, op1Reg, op3Reg, opt,

‎src/coreclr/jit/hwintrinsiccodegenxarch.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2623,7 +2623,8 @@ void CodeGen::genSse42Intrinsic(GenTreeHWIntrinsic* node, insOpts instOptions)
26232623
regNumber op1Reg = op1->GetRegNum();
26242624
GenTree* op2 = node->Op(2);
26252625

2626-
assert(!op2->isUsedFromReg() || (op2->GetRegNum() != targetReg) || (op1Reg == targetReg));
2626+
assert(!op2->isUsedFromReg() || (op2->GetRegNum() != targetReg) || (op1Reg == targetReg) ||
2627+
genIsSameLocalVar(op1, op2));
26272628
emit->emitIns_Mov(INS_mov, emitTypeSize(targetType), targetReg, op1Reg, /* canSkip */ true);
26282629

26292630
#ifdef TARGET_AMD64

0 commit comments

Comments
 (0)