[Arm64] Implement GT_XADD, GT_XCHG, GT_CMPXCHG ...#14329
[Arm64] Implement GT_XADD, GT_XCHG, GT_CMPXCHG ...#14329briansull merged 3 commits intodotnet:masterfrom
Conversation
f1ef1ff to
a6c542e
Compare
| BasicBlock* labelRetry = genCreateTempLabel(); | ||
| genDefineTempLabel(labelRetry); | ||
|
|
||
| getEmitter()->emitIns_R_R(INS_ldaxr, emitTypeSize(treeNode), loadReg, addrReg); |
There was a problem hiding this comment.
Are going to add this as a comment here?
There was a problem hiding this comment.
Opened an issue an added comments
| } | ||
| getEmitter()->emitInsBinary(ins, emitActualTypeSize(data), &i, data); | ||
|
|
||
| getEmitter()->emitIns_R_R_R(INS_stlxr, emitTypeSize(treeNode), exResultReg, storeDataReg, addrReg); |
| BasicBlock* labelCompareFail = genCreateTempLabel(); | ||
| genDefineTempLabel(labelRetry); | ||
|
|
||
| getEmitter()->emitIns_R_R(INS_ldaxr, emitTypeSize(treeNode), targetReg, addrReg); |
| getEmitter()->emitIns_J(INS_bne, labelCompareFail); | ||
| } | ||
|
|
||
| getEmitter()->emitIns_R_R_R(INS_stlxr, emitTypeSize(treeNode), exResultReg, dataReg, addrReg); |
|
I added barriers because I think they were implied by the existing implementation. I do not know if they are actually required |
|
test Windows_NT arm64 Cross Checked Build and Test |
| BasicBlock* labelRetry = genCreateTempLabel(); | ||
| genDefineTempLabel(labelRetry); | ||
|
|
||
| getEmitter()->emitIns_R_R(INS_ldaxr, emitTypeSize(treeNode), loadReg, addrReg); |
There was a problem hiding this comment.
Are going to add this as a comment here?
|
|
||
| // Store exclusive unpredictable cases must be avoided | ||
| assert(exResultReg != dataReg); | ||
| assert(exResultReg != addrReg); |
There was a problem hiding this comment.
You might want some/all of these cases to be noway_asserts.
As assert() only checks things in dbg/chk builds
There was a problem hiding this comment.
I changed them to asserts during development because I caught them failing and setting minopts
I set most of them back to noway_assert
| cmpXchgNode->gtOpLocation->gtLsraInfo.isDelayFree = true; | ||
| cmpXchgNode->gtOpValue->gtLsraInfo.isDelayFree = true; | ||
| if (!cmpXchgNode->gtOpComparand->isContained()) | ||
| cmpXchgNode->gtOpComparand->gtLsraInfo.isDelayFree = true; |
There was a problem hiding this comment.
you need { } for the then stmt, as per coding conventions.
| // it may be used used multiple during retries | ||
| tree->gtOp.gtOp1->gtLsraInfo.isDelayFree = true; | ||
| if (!tree->gtOp.gtOp2->isContained()) | ||
| tree->gtOp.gtOp2->gtLsraInfo.isDelayFree = true; |
There was a problem hiding this comment.
you need { } for the then stmt, as per coding conventions.
|
test Windows_NT arm64 Cross Checked Build and Test |
| info->hasDelayFreeSrc = true; | ||
|
|
||
| // Internals may not collide with target | ||
| info->isInternalRegDelayFree = true; |
There was a problem hiding this comment.
@CarolEidt might want to take a look and approve this section:
the lifetime of the addr and data must be extended because
it may be used used multiple during retries
CarolEidt
left a comment
There was a problem hiding this comment.
LGTM other than a couple of comment/format issues.
|
|
||
| #ifdef _TARGET_XARCH_ | ||
| #if defined(_TARGET_XARCH_) || defined(_TARGET_ARM64_) | ||
| // TODO-ARM-CQ: reenable treating Interlocked operation as intrinsic |
There was a problem hiding this comment.
Here, and below, the corresponding #endifs need to be updated.
| regNumber loadReg = (targetReg != REG_NA) ? targetReg : storeDataReg; | ||
|
|
||
| // The register allocator should have extended the lifetime of the address | ||
| // so that it is not used as the target. |
There was a problem hiding this comment.
Instead of addressing all of these requirements separately, it would be clearer, I think, to say:
// The register allocator should have extended the lifetimes of all input and internal registers so that
// none interfere with the target.
Done test Windows_NT arm64 Cross Checked Build and Test |
Fixes #14106
@dotnet/jit-contrib @dotnet/arm64-contrib PTAL