Skip to content

Conversation

@cbezault
Copy link
Contributor

@cbezault cbezault commented Dec 2, 2020

I'm not sure the alignas(16) on the __int128 is strictly necessary.
I'm also not 100% sure this is correct, though all the tests I ran on my Surface Pro X passed.

@BillyONeal @AlexGuteniev bat signals.

The codegen also seems right:
https://godbolt.org/z/1d8MnT vs

; Listing generated by Microsoft (R) Optimizing Compiler Version 19.28.29515.1

        TTL     C:\sources\test.obj
        ARM64

        AREA    |.drectve|, DRECTVE

        EXPORT  |?dest@@3RC_JC| [ DATA ]                ; dest

        AREA    |.data|, DATA
|?dest@@3RC_JC| DCQ 0x0, 0x0                            ;  = 0x0000000000000000 ; dest
        DCQ     0x1, 0x0                                ;  = 0x0000000000000001
        EXPORT  |main|

        AREA    |.pdata|, PDATA
|$pdata$main| DCD imagerel |$LN6|
        DCD     0x80004d
        ;Flags[SingleProEpi] functionLength[76] RegF[0] RegI[0] H[0] frameChainReturn[UnChained] frameSize[16]
; Function compile flags: /Odsp
; File C:\sources\test.cpp

        AREA    |.text$mn|, CODE, ARM64

|main|  PROC

; 6    : int main() {

|$LN6|
        sub         sp,sp,#0x10

; 7    :     __int64 compare[2];
; 8    :     _InterlockedCompareExchange128_nf(dest, compare[1], compare[0], compare);

        mov         x7,sp
        adrp        x8,|?dest@@3RC_JC|
        add         x15,x8,PageOffset(|?dest@@3RC_JC|)
        ldr         x14,[sp,#8]
        ldr         x13,[sp]
        ldp         x12,x11,[x7]
|$LN3@main|
        ldxp        x10,x9,[x15]
        cmp         x10,x12
        bne         |$LN4@main|
        cmp         x9,x11
        bne         |$LN4@main|
        stxp        w8,x13,x14,[x15]
        cmp         w8,#0
        bne         |$LN3@main|
|$LN4@main|
        stp         x10,x9,[x7]

; 9    :     return 0;

        mov         w0,#0
        add         sp,sp,#0x10
        ret

        ENDP  ; |main|

        END

@cbezault cbezault requested a review from a team as a code owner December 2, 2020 00:41
@cbezault cbezault added ARM64 Related to the ARM64 architecture bug Something isn't working labels Dec 2, 2020
@cbezault
Copy link
Contributor Author

cbezault commented Dec 2, 2020

Maybe fixes #1491

cbezault and others added 2 commits December 1, 2020 21:02
Co-authored-by: Casey Carter <cartec69@gmail.com>
@cbezault
Copy link
Contributor Author

cbezault commented Dec 2, 2020

Okay went through all the steps of actually compiling the STL and then a test program and testing it on the Surface Pro X again. I also looked at the new codegen and it seems right.

... and make `_Dest` be `const` while I'm here.
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I wanted to apply a small refactor to reduce duplication in the not-clang and not-ARM64 cases, which quickly turned into restoring the original code on the #else branch. I also made _Dest be const in the workaround path.

@StephanTLavavej StephanTLavavej removed their assignment Dec 3, 2020
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej StephanTLavavej self-assigned this Dec 3, 2020
@StephanTLavavej StephanTLavavej merged commit 076863c into microsoft:master Dec 3, 2020
@StephanTLavavej
Copy link
Member

Thanks for fixing and validating this! 🛠️ 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARM64 Related to the ARM64 architecture bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<atomic>: clang-cl reports undeclared identifier '_InterlockedCompareExchange128_nf' for ARM64

3 participants