[LoongArch64] Add atomic instruction implementation for LoongArch64 - #129683
[LoongArch64] Add atomic instruction implementation for LoongArch64#129683lawn123 wants to merge 23 commits into
Conversation
|
Hi @shushanhf @LuckyXu-HF @jakobbotsch @jkotas could you please review this PR? Thanks. |
| /// <para>AMSWAP[_DB].H rd, rk, rj</para> | ||
| /// <para>This is the newly added atomic instruction on ISA1.1</para> | ||
| /// </summary> | ||
| internal static ushort Exchange(ref ushort location1, ushort value) => Exchange(ref location1, value); |
There was a problem hiding this comment.
How does Exchange work for int and long that it is missing here?
There was a problem hiding this comment.
For int and long types Exchange supports both V1.0 and V1.1.
7b904f6 to
e4cdfee
Compare
Change-Id: I3b7757ecfeff9aba28407508b196d55b4e21b69d
Change-Id: Ib826fc3ffb9057835357b68a51e20da66f9cebd5
Change-Id: Id3be930d58440bbc39b307a211a9fcb444f1e3ca
…on and opportunistics light-up for NAOT Change-Id: I419341c499d0c4e955a60e5f4ad6fdeef74f9277
…rmatting issues Change-Id: I1b3d1010ac3642624ec1f66eb26dc11220460d7a
…efault and revert System.Runtime.Intrinsics.cs changes. Change-Id: If9f10ec7908aaf706b1d2ad9f2e34db073543d36
Change-Id: I2c1e8d43106616da47e231670e557f508330e3d8
…n.sh Change-Id: I3240c5d2413950981b4f1446d837d9c0de4e9a65
Change-Id: I51fe7faac05749bc38344e2068bb2efccc1ce99a
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. LoongArch64 gained hardware atomic instructions (AMSWAP/AMADD/AMAND/AMOR in ISA1.0 and byte/halfword AM*.B/.H plus AMCAS in ISA1.1). Previously the JIT left Interlocked codegen as NYI and fell back to LL/SC or managed emulation. Wiring up the real atomic instructions is a legitimate CQ improvement, and the author's benchmark shows ~1.2x speedups on small-type CompareExchange.
Approach: Sound and consistent with the codebase. The change follows the established RISCV64/ARM64 pattern end-to-end: a new LoongArch64 ISA arch with two feature sets (LAM_BH, LAM_CAS) threaded through InstructionSetDesc.txt, the R2R/JIT-EE enums, CLR/JIT config knobs, minipal_getcpufeatures CPUCFG probing, NativeAOT g_cpuFeatures, the System.Runtime.Intrinsics.LoongArch.LAM intrinsic surface, and JIT importer/lowering/LSRA/codegen/emit. Runtime capability is detected dynamically with a correct LL/SC fallback when LAM_CAS is unavailable, and small-type paths throw PlatformNotSupported/fall back to managed emulation when LAM_BH is unavailable.
Summary: _db barrier variants vs. explicit instGen_MemoryBarrier), and the LL/SC retry loop. Also note the new internal LAM/LAM.BH/LAM.CAS types are non-public intrinsics (no ref/ public API surface), so no API review is required. One low-severity defensive-coding suggestion is filed inline; the open maintainer question about whether the hand-rolled small-type emulation is worth the added codegen complexity (vs. only expanding when LAM_CAS/LAM_BH is present) should be resolved before merge.
Detailed Findings
⚠️ Codegen — small-type XCHG defensive gap
See the inline comment on genLockedInstructions (src/coreclr/jit/codegenloongarch64.cpp). The small-type GT_XCHG branch emits nothing if LAM_BH is absent, leaving targetReg undefined. It is unreachable given the importer gating, but an assert/unreached() would make the invariant explicit and match the style already used in genCodeForCmpXchg.
💡 Consistency — minor comment/style nits already raised by maintainers
clrconfigvalues.h: theEnableLoongArch64LAM_CASdescription still readsLAM_BH+(copy/paste); it should describeLAM_CAS. Already flagged by a maintainer.cpufeatures.c: the localLAM_BH/LAM_CAS#defines and theworld/cpucfgnaming are a bit ad hoc; consider a comment referencing the CPUCFG word index (word 2, bits for AMCAS/AM*.B.H) for future readers. Non-blocking.
✅ ISA plumbing — correct and complete
The InstructionSetDesc.txt additions (R2R bits 94/95, NEXT_AVAILABLE_R2R_BIT = 96), hwintrinsicIsaRangeArray, ReadyToRunInstructionSetHelper, HardwareIntrinsicHelpers, InstructionSetHelpers optimistic set, and methodtablebuilder.cpp namespace mapping are all internally consistent with each other and with how other arches register feature sets. The jiteeversionguid.h bump is present as required for a JIT-EE interface change.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 228.3 AIC · ⌖ 15.3 AIC · ⊞ 10K
| ins = INS_amswap_db_h; | ||
| } | ||
| emit->emitIns_R_R_R(ins, dataSize, targetReg, dataReg, addrReg); | ||
| } |
There was a problem hiding this comment.
💡 In this small-type GT_XCHG branch, if InstructionSet_LAM_BH is not supported the if is skipped, ins remains INS_none, and no instruction is emitted at all — targetReg is left undefined and the swap silently doesn't happen. In practice this path is unreachable because impIntrinsic only expands small-type Exchange when LAM_BH is available (otherwise it falls back to the managed impl or throws PlatformNotSupported), so this is defensive rather than a live bug. Still, consider adding an else { unreached(); } / assert(HasInstructionSet(InstructionSet_LAM_BH)) before the block so an invariant violation fails loudly instead of emitting a silent no-op. This mirrors the explicit asserts you already added in genCodeForCmpXchg.
Resolve conflicts and set assert when InstructionSet_LAM_SH is not supported in genLockedInstructions. Change-Id: I70070881a49fc3335c549f55e67ad3445a775c54
|
HI @jakobbotsch could you please review this PR when you have time. |
There was a problem hiding this comment.
Holistic Review
Motivation: Unchanged and still justified. LoongArch64 gained hardware atomic instructions (AMSWAP/AMADD/AMAND/AMOR in ISA1.0 and byte/halfword AM*.B/.H plus AMCAS in ISA1.1). Previously the JIT left Interlocked codegen as NYI and fell back to LL/SC or managed emulation. Wiring up the real atomic instructions is a legitimate CQ improvement.
Approach: Unchanged and sound. The change follows the established RISCV64/ARM64 pattern end-to-end: a new LoongArch64 ISA arch with two feature sets (LAM_BH, LAM_CAS) threaded through InstructionSetDesc.txt, the R2R/JIT-EE enums, CLR/JIT config knobs, minipal_getcpufeatures CPUCFG probing, NativeAOT g_cpuFeatures, the System.Runtime.Intrinsics.LoongArch.LAM intrinsic surface, and JIT importer/lowering/LSRA/codegen/emit. Runtime capability is detected dynamically with a correct LL/SC fallback when LAM_CAS is unavailable.
Summary: main; the 12 PR commits are byte-identical in the range-diff. The only incremental content changes are mechanical and correct rebase artifacts: (1) the JIT-EE version GUID in jiteeversionguid.h was regenerated, and (2) the R2R instruction-set bit numbering for LAM_BH/LAM_CAS was shifted from 94/95 to 95/96 to avoid colliding with the upstream-added Zicond=94. I verified this renumbering is internally consistent across readytoruninstructionset.h, ReadyToRunInstructionSet.cs, CorInfoInstructionSet.cs, and ReadyToRunInstructionSetHelper.cs at the current head. No new actionable findings arise from the incremental changes. The target-specific codegen concerns from the initial review (atomic encodings, _db memory-ordering variants, the LL/SC retry loop, and the small-type emulation complexity question) remain and still warrant confirmation by a maintainer with LA64 expertise; those are in unchanged code and are not re-raised as new findings here.
Assessment History
- review 4730679920 reviewed commit
02f0815b93a109f8d85b019820d18061feca9576with verdict⚠️ Needs Human Review. Current verdict is⚠️ Needs Human Review — unchanged. The motivation, approach, and risk assessment are all unchanged; the intervening rebase produced only a JIT-EE GUID regeneration and a rebase-driven R2R bit renumbering, neither of which alters the PR's behavior or risk.
Detailed Findings
No new actionable findings in the incremental (rebase) scope. The prior review's inline suggestion and open maintainer questions still apply to the unchanged PR patches.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 69.1 AIC · ⌖ 14.8 AIC · ⊞ 10K
Change-Id: I918e7509790310ac6c7d675ae638d6567c60a5d3
|
@lawn123 could you fix the jit-format failure? That one is at the very least a blocker. |
Updated. |
There was a problem hiding this comment.
Holistic Review
Motivation: Unchanged and still justified. LoongArch64 gained hardware atomic instructions (AMSWAP/AMADD/AMAND/AMOR in ISA1.0 and byte/halfword AM*.B/.H plus AMCAS in ISA1.1). Previously the JIT left Interlocked codegen as NYI and fell back to LL/SC or managed emulation. Wiring up the real atomic instructions is a legitimate CQ improvement.
Approach: Unchanged and sound. The change follows the established RISCV64/ARM64 pattern end-to-end: a new LoongArch64 ISA arch with two feature sets (LAM_BH, LAM_CAS) threaded through InstructionSetDesc.txt, the R2R/JIT-EE enums, CLR/JIT config knobs, minipal_getcpufeatures CPUCFG probing, NativeAOT g_cpuFeatures, the System.Runtime.Intrinsics.LoongArch.LAM intrinsic surface, and JIT importer/lowering/LSRA/codegen/emit. Runtime capability is detected dynamically with a correct LL/SC fallback when LAM_CAS is unavailable.
Summary: 534fe40e, "fix jit-format failure") that mechanically reflows two lines in src/coreclr/jit/rangecheck.h to satisfy jit-format's column/wrapping rules. It is a pure whitespace/formatting change with no behavioral effect: the r1StraddlesZero/r2StraddlesZero computations and the signedEndpointOverflows guard are logically identical to before, just re-wrapped. The 12 previously-reviewed PR commits are byte-identical in the range-diff and the merge base is unchanged. No new actionable findings arise from this incremental change. The target-specific codegen concerns from the initial review (atomic encodings, _db memory-ordering variants, the LL/SC retry loop, and the small-type emulation complexity question) remain in unchanged code and still warrant confirmation by a maintainer with LA64 expertise; they are not re-raised as new findings here.
Assessment History
- review 4730679920 reviewed commit
02f0815b93a109f8d85b019820d18061feca9576with verdict⚠️ Needs Human Review. Current verdict is⚠️ Needs Human Review — unchanged. Motivation, approach, and risk assessment are all unchanged; subsequent changes are a rebase and a formatting-only commit that do not alter behavior or risk. - review 4732014090 reviewed commit
9620eb16ca9a2724e3df3b3245ea836f2ac82151with verdict⚠️ Needs Human Review. Current verdict is⚠️ Needs Human Review — unchanged. The only new content since that review is the jit-format whitespace reflow inrangecheck.h, which has no behavioral impact.
Detailed Findings
No new actionable findings in the incremental scope. The sole incremental change is a jit-format whitespace reflow in src/coreclr/jit/rangecheck.h. The prior review's inline suggestion and open maintainer questions still apply to the unchanged PR patches.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 51.7 AIC · ⌖ 15.7 AIC · ⊞ 10K
Change-Id: Id733a21689d77c295b7c1c98ef9d46b3a9e98251
|
HI @jakobbotsch @MichalStrehovsky this PR has been open for a few days. Could you please take a look when you have a chance? Thanks! |
| else if (varTypeIsShort(treeNode->TypeGet())) | ||
| { | ||
| ins = INS_amswap_db_h; | ||
| } |
There was a problem hiding this comment.
| else if (varTypeIsShort(treeNode->TypeGet())) | |
| { | |
| ins = INS_amswap_db_h; | |
| } | |
| else | |
| { | |
| assert(varTypeIsShort(treeNode)); | |
| ins = INS_amswap_db_h; | |
| } |
| #if defined(TARGET_LOONGARCH64) | ||
| case NI_System_Threading_Interlocked_CompareExchange: | ||
| case NI_System_Threading_Interlocked_Exchange: | ||
| betterToExpand = true; | ||
| break; | ||
| #endif |
There was a problem hiding this comment.
| #if defined(TARGET_LOONGARCH64) | |
| case NI_System_Threading_Interlocked_CompareExchange: | |
| case NI_System_Threading_Interlocked_Exchange: | |
| betterToExpand = true; | |
| break; | |
| #endif |
These are already marked betterToExpand above that kicks in for both NativeAOT and CoreCLR when optimizing.
jakobbotsch
left a comment
There was a problem hiding this comment.
The JIT codegen/import looks ok to me. @tannergooding can you give a final approval for all the intrinsic-related changes in the different layers?
Change-Id: I5604be377c6451c96da56b8f55c6d126fe8f6a9d
@jakobbotsch Thank you. I have updated according to your suggestion. @tannergooding Could you please take a look when you have a chance? Thanks! |
Change-Id: Ie26a7a659ca6087ee06972364d727fbe58650662
|
#129683 (review) |
|
Hi @tannergooding, |
Change-Id: I4d33b5ea6b8fae5a3cd7c620fb8563726269f1f1
Change-Id: Ic95bd9d0954006d3d23663e636c40dda445febfa
Hi @tannergooding, |
|
Sorry for the delay, this is on my backlog but I've been busy working on things that must make the .NET 11 P7 snap. |
Understood, thanks for letting me know! I'll wait for your review when things settle down. |
Add atomic instruction for LoongArch64. Supports atomic instructions on ISA1.0 and ISA1.1
Implementation of #122745.