[NativeAOT] Use 8.1 atomics, if available, in RhpCheckedXchg/RhpCheckedLockCmpXchg #85283
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsWe could, in theory, test for presence of LSE instructions dynamically, - set a global flag early on, and check it when need to do an object exchange. I am not sure if potential improvement could be more than the cost of the check. When it is guaranteed that LSE is present, it is a trivial change.
|
The improvement is more than the cost of the check based on what we have seen earlier. We use the dynamic check for standard CoreCLR: runtime/src/coreclr/vm/util.hpp Line 95 in 11a0671 |
|
I have added a dynamic check when |
| @@ -81,6 +81,10 @@ TrapThreadsFlags_TrapThreads equ 2 | |||
| TrapThreadsFlags_AbortInProgress_Bit equ 0 | |||
| TrapThreadsFlags_TrapThreads_Bit equ 1 | |||
|
|
|||
| ;; Bit position for the ARM64IntrinsicConstants_Atomics flags, to be used with tbz / tbnz instructions | |||
| ;; ARM64IntrinsicConstants_Atomics = 0x0080 | |||
| ARM64_ATOMICS_FEATURE_FLAG_BIT equ 7 | |||
There was a problem hiding this comment.
Can we validate that the value of this constant matches the C++ version?
This looks very likely to get forgotten if we ever shuffle the enum for whatever reason.
We have a mechanism for compile-time validation of these things in AsmOffsets.h. It's not an offset per se, but this is used to ensure MAX_STRING_LENGTH agree, etc.
There was a problem hiding this comment.
I'll put this into offsets (with appropriate check).
There was a problem hiding this comment.
ARM64_ATOMICS_FEATURE_FLAG_BIT is in the AsmOffsets.h now and there is an assert that couples it with ARM64IntrinsicConstants_Atomics
|
hmm arm64 asm on linux is not happy about LSE instructions. I wonder if LSE can be enabled locally |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -230,12 +230,21 @@ int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, pint_t pc, | |||
| p &= ~0xfULL; | |||
| // CFA is the bottom of the current stack frame. | |||
| for (; p < cfa; p += 16) { | |||
| #ifdef __GNUC__ | |||
There was a problem hiding this comment.
clang defines __GNUC__ for compat, so this is #if 1 for all practical purposes.
There was a problem hiding this comment.
hmm, that is not very useful. Is there a way to detect GCC, or do we ever compile this file with GCC?
There was a problem hiding this comment.
GCC does not document mte as something it would accept.
There was a problem hiding this comment.
We can just put #if 0 around this whole block for now. We do not need support for mtag in our copy of libunwind.
There was a problem hiding this comment.
Although it might be that both clang and gcc support mte:
There was a problem hiding this comment.
Is there a way to detect GCC
We can instead use #ifdef __llvm__ (if it is really needed)
Please reference the commit modifying external source in src/native/external/llvm-libunwind-version.txt.
There was a problem hiding this comment.
The problem is with older versions of llvm that do not understand this extension at all.
There was a problem hiding this comment.
Maybe just replace it with assert(false) until we have to support ‘mte’?
We’d have to switch to compiler that supports it by then.
There was a problem hiding this comment.
This is a dead code as we are setting stage2 to false: https://github.com/dotnet/runtime/pull/85179/files#diff-342f897208801d38daad0d3b917fae03bdcc260357ebfc132231be828977dae0 and this code is under if (stage2 && ..) condition, so we can just skip this branch from compilation as @jkotas has done in #85431.
ghost
locked as resolved and limited conversation to collaborators
We could, in theory, test for presence of LSE instructions dynamically, - set a global flag early on, and check it when need to do an object exchange. I am not sure if potential improvement could be more than the cost of the check.
When it is guaranteed that LSE is present, it is a trivial change.