Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Use half-fences for volatile loads/stores on Windows ARM64 #27384

Merged
merged 2 commits into from
Oct 25, 2019
Merged

Use half-fences for volatile loads/stores on Windows ARM64 #27384

merged 2 commits into from
Oct 25, 2019

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Oct 23, 2019 •
edited
Loading

Needed for #23548

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

In particular the perf sensitive part of TryGet from #23548

Before:

00000001`800f0c6c 9b0831c8 madd    x8, x14, x8, x12
00000001`800f0c70 9100a10a add     x10, x8, #0x28
00000001`800f0c74 b9400149 ldr     w9, [x10]
00000001`800f0c78 d5033fbf dmb     sy                             <---
00000001`800f0c7c f9400548 ldr     x8, [x10, #8]
00000001`800f0c80 eb00011f cmp     x8, x0
00000001`800f0c84 540000c1 bne     coreclr!CastCache::TryGet+0x64 (00000001`800f0c9c)
00000001`800f0c88 f9400948 ldr     x8, [x10, #0x10]
00000001`800f0c8c d5033fbf dmb     sy                             <---
00000001`800f0c90 ca01010b eor     x11, x8, x1
00000001`800f0c94 f100057f cmp     x11, #1

After:

00000001`800f0674 9b0831e8 madd    x8, x15, x8, x12
00000001`800f0678 9100a10b add     x11, x8, #0x28
00000001`800f067c 88dffd6a ldar    w10, [x11]                <---
00000001`800f0680 f9400568 ldr     x8, [x11, #8]
00000001`800f0684 eb00011f cmp     x8, x0
00000001`800f0688 540000c1 bne     coreclr!CastCache::TryGet+0x60 (00000001`800f06a0)
00000001`800f068c 91004168 add     x8, x11, #0x10
00000001`800f0690 c8dffd09 ldar    x9, [x8]                  <---
00000001`800f0694 ca01012d eor     x13, x9, x1
00000001`800f0698 f10005bf cmp     x13, #1

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

CC @jkotas @davidwrighton @sdmaclea

@jkotas
Copy link
Member

jkotas commented Oct 23, 2019 

f:\workspace\_work\1\s\src\inc\volatile.h(163): error C3861: '__ldar16': identifier not found [F:\workspace\_work\1\s\bin\obj\Windows_NT.arm64.Checked\src\debug\ee\wks\cordbee_wks.vcxproj]
f:\workspace\_work\1\s\src\inc\volatile.h(166): error C3861: '__ldar32': identifier not found [F:\workspace\_work\1\s\bin\obj\Windows_NT.arm64.Checked\src\debug\ee\wks\cordbee_wks.vcxproj]
f:\workspace\_work\1\s\src\inc\volatile.h(169): error C3861: '__ldar64': identifier not found [F:\workspace\_work\1\s\bin\obj\Windows_NT.arm64.Checked\src\debug\ee\wks\cordbee_wks.vcxproj]

@stephentoub
Copy link
Member

Related: https://github.com/dotnet/coreclr/issues/11545

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

Build machine uses VS2017 ?

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

https://github.com/dotnet/coreclr/issues/11545 Is related. However this is not about JIT-ed code. This tweaks c++ Volatile helpers when MSVC is used.
We do the right thing on GCC already.

@sdmaclea
Copy link

Possibly related #11885

@sdmaclea sdmaclea added this to the 5.0 milestone Oct 23, 2019
sdmaclea
sdmaclea approved these changes Oct 23, 2019
View reviewed changes
Copy link

@sdmaclea sdmaclea left a comment

Choose a reason for hiding this comment

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

LGTM if you can fix to compilation issues.

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

Yes, we are using VS2017 in the build.

BUILD: Commencing build of cross architecture native components for Windows_NT.arm64.Release
BUILD: Using environment: "C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\Tools\\..\..\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64
**********************************************************************
** Visual Studio 2017 Developer Command Prompt v15.9.16
** Copyright (c) 2017 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x86_x64'
-- Selecting Windows SDK version 10.0.17763.0 to target Windows 10.0.
-- The C compiler identification is MSVC 19.16.27034.0
-- The CXX compiler identification is MSVC 19.16.27034.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe -- works

@MichalStrehovsky
Copy link
Member

Should this also be replicated in GC's copy of volatile.h under src\gc\env?

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

Our build instructions require VS2019. https://github.com/dotnet/coreclr/blob/master/Documentation/building/windows-instructions.md

Why the lab uses 2017? How can this be fixed?

@sdmaclea
Copy link

Our build instructions require VS2019. https://github.com/dotnet/coreclr/blob/master/Documentation/building/windows-instructions.md

Why the lab uses 2017? How can this be fixed?

/cc @jashook

@VSadov
Copy link
Member Author

VSadov commented Oct 23, 2019

Yes. gc\env\volatile.h also needs updating. I did not realize it is a copy. Thanks!

@janvorli
Copy link
Member

Why the lab uses 2017? How can this be fixed?

There was a problem with optimizations in VS2019 breaking our explicit frames stuff during EH. Release builds would crash when exception was thrown from some places in the runtime. While I've fixed that some time ago, @jkotas has discovered there is still an issue that needs to be fixed before we can migrate our builds to VS2019 (#27247).

@jkotas
Copy link
Member

jkotas commented Oct 23, 2019

there is still an issue that needs to be fixed before we can migrate our builds to VS2019 (#27247).

This issue should not be blocking upgrade to VS2019.

@janvorli
Copy link
Member

This issue should not be blocking upgrade to VS2019.

You are right, the issue now exists with both VS2019 and 2017.

@VSadov VSadov changed the title Use half-fences for volatile load/stores on Windows ARM64 Use half-fences for volatile loads/stores on Windows ARM64 Oct 25, 2019
@VSadov
Copy link
Member Author

VSadov commented Oct 25, 2019

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member Author

VSadov commented Oct 25, 2019

outerloop tests passed on Windows ARM64.

@jkotas jkotas merged commit c128dba into dotnet:master Oct 25, 2019
@VSadov
Copy link
Member Author

VSadov commented Oct 25, 2019

Thanks!!

@BruceForstall
Copy link
Member

@VSadov You state here,

outerloop tests passed on Windows ARM64.

However, the CI system currently does not run any tests on Windows arm64. If you look at the jobs, the important "Send tests to Helix" step is skipped for the two Windows arm64 Pri1 test runs.

So it seems this change got zero testing in the CI.

@VSadov
Copy link
Member Author

VSadov commented Nov 13, 2019

I thought we skip running in inner loop, but that outer loop does some running on Win-arm64

@VSadov
Copy link
Member Author

VSadov commented Nov 13, 2019

Also the changes make windows to use the same fences in Volatile as was used on Unix for a while.
Do we have the issues on Windows only or on Unix as well?

@VSadov
Copy link
Member Author

VSadov commented Nov 13, 2019

Oh, I see it is the type switch that could turn float into int.

Now, if we do a fix - how can we have any CI coverage for the fix?

@BruceForstall
Copy link
Member

how can we have any CI coverage for the fix?

There is currently no support for any Windows arm64 testing in the CI today. Talk to @jashook about when we might get some... hopefully soon.

@jashook
Copy link

jashook commented Nov 13, 2019 •
edited
Loading

@VSadov for your change only please remove the skip for helix testing on arm64

MichalStrehovsky pushed a commit to MichalStrehovsky/corert that referenced this pull request Mar 28, 2020
@VSadov @MichalStrehovsky
Use half-fences for volatile loads/stores on Windows ARM64 (dotnet/co…
6994b81 
…reclr#27384)

* Use half-fences for volatile load/stores on Windows ARM64

* Updated Volatile.h in gc/env as well.
unified on type names and warning suppression.

Commit migrated from dotnet/coreclr@c128dba
MichalStrehovsky pushed a commit to MichalStrehovsky/corert that referenced this pull request Mar 31, 2020
@VSadov @MichalStrehovsky
Use half-fences for volatile loads/stores on Windows ARM64 (dotnet/co…
e39f88b 
…reclr#27384)

* Use half-fences for volatile load/stores on Windows ARM64

* Updated Volatile.h in gc/env as well.
unified on type names and warning suppression.

Commit migrated from dotnet/coreclr@c128dba
jkotas pushed a commit to dotnet/corert that referenced this pull request Apr 1, 2020
@VSadov @jkotas
Use half-fences for volatile loads/stores on Windows ARM64 (dotnet/co…
2672d9c 
…reclr#27384)

* Use half-fences for volatile load/stores on Windows ARM64

* Updated Volatile.h in gc/env as well.
unified on type names and warning suppression.

Commit migrated from dotnet/coreclr@c128dba
@briansull briansull mentioned this pull request Jun 30, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@sdmaclea sdmaclea sdmaclea approved these changes

Assignees
No one assigned
Labels
arch-arm64 os-windows
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
8 participants
@VSadov @jkotas @stephentoub @sdmaclea @MichalStrehovsky @janvorli @BruceForstall @jashook