[SuperPMI] Support spmi-asmdiffs on Arm64 #48683

echesakov · 2021-02-24T04:49:20Z

Convert #ifdef TARGET_* in CompileResult::applyRelocs in compileresult.cpp to runtime checks and allow SuperPMI to work with cross-architecture jits during asmdiffs (e.g. clrjit_win_arm64_x64.dll)
Refactor CompileResult::applyRelocs and separate platform specific relocations (x86, arm64, 64-bit targets) from platform agnostic relocations
Support IMAGE_REL_ARM64_BRANCH26, IMAGE_REL_ARM64_PAGEBASE_REL21 and IMAGE_REL_ARM64_PAGEOFFSET_12A in CompileResult::applyRelocs.
Support IMAGE_REL_BASED_THUMB_MOV32, IMAGE_REL_BASED_REL_THUMB_MOV32_PCREL and IMAGE_REL_BASED_THUMB_BRANCH24 in CompileResult::applyRelocs.
Adjust code section size that SuperPMI gets by calling repAllocMem in NearDiffer::compare in neardiffer.cpp. On Arm64 methods that have literal (constant) pools will fail decoding since the pools are appended to the methods code section and the size of code section reported by repAllocMem will include the constant pool. In order to avoid decoding of the constant pools and adjust code section to only contain executable portion - use repCompileMethod in NearDiffer::compare.

… and move them to src/coreclr/ToolBox/superpmi/superpmi-shared

…ompileResult::applyRelocs in src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

…r/ToolBox/superpmi/superpmi-shared/compileresult.cpp

…superpmi/superpmi-shared/compileresult.cpp

…n src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

…i/superpmi-shared/compileresult.cpp

…/ToolBox/superpmi/superpmi/neardiffer.cpp

…red/spmiutil.cpp src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.h

…perpmi/superpmi-shared/spmiutil.cpp src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.h

…AGEBASE_REL21 and IMAGE_REL_ARM64_PAGEOFFSET_12A in compileresult.cpp

…x/superpmi/superpmi-shared/spmiutil.cpp src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.h

…_PCREL IMAGE_REL_BASED_THUMB_BRANCH24 on Arm in compileresult.cpp

echesakov · 2021-02-25T21:58:59Z

src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

+                    if ((section_begin <= address) && (address < section_end)) // A reloc for our section?
+                    {
+                        if (!FitsInThumb2BlRel24(delta))
+                        {


I was not sure what to do here. IIRC, if such overflow occurs in real product during crossgen the JIT would be required to re-compile the method without using this type of relocation.

@BruceForstall Any opinion if this is desired behavior? What would/should happen during spmi collect when such relocation overflow occurs? Would the corresponding method context be included in the final collection?

I'm not sure what would happen during spmi collection if it failed. It's supposed to throw and retry, but the retry logic for relocs in crossgen1 seems to be dependent on m_fNGenLastRetry, but I can't find how that ever gets set to true (because it's dependent on fNgenLastRetry, which is never set to true).

It looks like crossgen2 always returns IMAGE_REL_BASED_THUMB_BRANCH24 with no provision for overflow.

I would guess spmi would only record the retry compilation, but it's not clear that ever happens.

echesakov · 2021-02-25T22:36:18Z

With this change I could successfully run spmi-asmdiffs on

win-x64 host using clrjit_win_arm64_x64.dll with the latest win-arm64 mch collections
win-x86 host using clrjit_unix_arm_x86.dll with the latest linux-arm mch collections

No errors due to "Unknown reloc type" or "Decoding has failed"

Note that I had to remove this line that was setting JitRequired=1 during superpmi.py asmdiffs (but not during replay). Otherwise, spmi-asmdiffs would trigger assertions in the JIT due to BADCODE while replaying some of the tests (e.g. JIT\Directed\coverage\importer\Desktop\badldsfld_il_r). After discussion this with Bruce - we came to conclusion that the line should be removed from the script.

@BruceForstall @dotnet/jit-contrib PTAL

BruceForstall

There are a couple other TARGET ifdefs it seems like you could address while you're at it:

TypeUtils::GetCorInfoTypeName() // simple output code
CallUtils::HasRetBuffArg() // unreferenced; can just be deleted

src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.cpp

src/coreclr/ToolBox/superpmi/superpmi/neardiffer.cpp

src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

src/coreclr/ToolBox/superpmi/superpmi/jitinstance.cpp

…Arm64 in neardiffer.cpp

…sult.cpp

…perpmi/superpmi-shared/compileresult.cpp

…il.h

BruceForstall

LGTM. Thanks for finding the sizeof() cases. I'm sure we'll find more cross-target issues over time, but this is a good update.

src/coreclr/ToolBox/superpmi/superpmi/neardiffer.cpp

BruceForstall · 2021-02-26T06:17:57Z

src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

+                    if ((section_begin <= address) && (address < section_end)) // A reloc for our section?
+                    {
+                        if (!FitsInThumb2BlRel24(delta))
+                        {


I'm not sure what would happen during spmi collection if it failed. It's supposed to throw and retry, but the retry logic for relocs in crossgen1 seems to be dependent on m_fNGenLastRetry, but I can't find how that ever gets set to true (because it's dependent on fNgenLastRetry, which is never set to true).

It looks like crossgen2 always returns IMAGE_REL_BASED_THUMB_BRANCH24 with no provision for overflow.

I would guess spmi would only record the retry compilation, but it's not clear that ever happens.

ghost · 2021-02-26T19:02:23Z

Hello @echesakovMSFT!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

echesakov · 2021-02-26T22:32:27Z

The failure in runtime (CoreCLR Pri0 Runtime Tests Run windows arm64 checked) is not relevant

D:\h\w\B2C309DA\w\A26B08C8\e>dotnet D:\h\w\B2C309DA\p\xunit\xunit.console.dll JIT\HardwareIntrinsics\JIT.HardwareIntrinsics.XUnitWrapper.dll -parallel collections -nocolor -noshadow -xml testResults.xml -trait TestGroup=JIT.HardwareIntrinsics 
Microsoft.DotNet.XUnitConsoleRunner v2.5.0 (64-bit .NET 5.0.0)
System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.
System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.

tracked by #48820

echesakov added 9 commits February 23, 2021 20:13

Implement IMAGE_REL_ARM64_BRANCH26 in compileresult.cpp

35f7bf1

Introduce GetSpmiTargetArchitecture/SetSpmiTargetArchitecture helpers…

80be600

… and move them to src/coreclr/ToolBox/superpmi/superpmi-shared

Do platform specific relocations (IMAGE_REL_BASED_HIGHLOW) first in C…

bbd037f

…ompileResult::applyRelocs in src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

Do Arm64 specific relocations first (IMAGE_REL_ARM64_*) in src/corecl…

f5ec6d2

…r/ToolBox/superpmi/superpmi-shared/compileresult.cpp

Rename 'relocWasHandled' -> 'wasRelocHandled' in src/coreclr/ToolBox/…

226acee

…superpmi/superpmi-shared/compileresult.cpp

Do 64-bit target specific relocations first (IMAGE_REL_BASED_DIR64) i…

a4cce85

…n src/coreclr/ToolBox/superpmi/superpmi-shared/compileresult.cpp

Remove remaning 'if defined(TARGET_*)' in src/coreclr/ToolBox/superpm…

bd38deb

…i/superpmi-shared/compileresult.cpp

Call repCompileMethod() on Arm64 to adjust hotCodeSize in src/coreclr…

1e31f44

…/ToolBox/superpmi/superpmi/neardiffer.cpp

Add PutArm64Rel28 helper in src/coreclr/ToolBox/superpmi/superpmi-sha…

72c97ba

…red/spmiutil.cpp src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.h

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 24, 2021

echesakov changed the title ~~[SuperPMI] Add support for Arm64~~ [SuperPMI] Support spmi-asmdiffs on Arm64 Feb 24, 2021

echesakov added the arch-arm64 label Feb 24, 2021

echesakov added 4 commits February 24, 2021 16:16

Add PutArm64Rel21 and PutArm64Rel12 helpers in src/coreclr/ToolBox/su…

48aff18

…perpmi/superpmi-shared/spmiutil.cpp src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.h

Re-implement IMAGE_REL_ARM64_BRANCH26 and implement IMAGE_REL_ARM64_P…

ae3fa9d

…AGEBASE_REL21 and IMAGE_REL_ARM64_PAGEOFFSET_12A in compileresult.cpp

Add PutThumb2Mov32 and PutThumb2BlRel24 helpers in src/coreclr/ToolBo…

a0b8cb4

…x/superpmi/superpmi-shared/spmiutil.cpp src/coreclr/ToolBox/superpmi/superpmi-shared/spmiutil.h

Enable IMAGE_REL_BASED_HIGHLOW on Arm in compileresult.cpp

0a763b4

runfoapp bot mentioned this pull request Feb 25, 2021

NuGet restore failure reading feed information #43696

Closed

Implement IMAGE_REL_BASED_THUMB_MOV32 IMAGE_REL_BASED_REL_THUMB_MOV32…

30023dd

…_PCREL IMAGE_REL_BASED_THUMB_BRANCH24 on Arm in compileresult.cpp

echesakov force-pushed the SuperPMI-Arm64 branch from 4fa622b to 30023dd Compare February 25, 2021 18:56

echesakov commented Feb 25, 2021

View reviewed changes

echesakov marked this pull request as ready for review February 25, 2021 21:59

echesakov self-assigned this Feb 25, 2021

echesakov added the arch-arm32 label Feb 25, 2021

echesakov mentioned this pull request Feb 25, 2021

Separate refactoring changes in 43250 #48199

Merged

BruceForstall requested changes Feb 25, 2021

View reviewed changes

echesakov added 2 commits February 25, 2021 17:59

Add comment about functions copied from utilcode in spmiutil.cpp

6fe2d46

Generalize checks for altjits in jitinstance.cpp

df78f9c

echesakov added 10 commits February 25, 2021 19:39

Add comment and add logic for figuring out roDataBlock/roDataSize on …

b9c6214

…Arm64 in neardiffer.cpp

Remove redundant cast in compileresult.cpp

b88159d

Use one error message for both 32-bit and 64-bit targets in compilere…

13d16ea

…sult.cpp

Add comment before all platform relocations in src/coreclr/ToolBox/su…

227d6f6

…perpmi/superpmi-shared/compileresult.cpp

Remove another redundant cast in compileresult.cpp

ac3ce2e

Remove unused CallUtils::HasRetBuffArg in callutils.cpp callutils.h

365b24a

Add IsSpmiTarget64Bit() and SpmiTargetPointerSize() helpers in spmiut…

f805314

…il.h

Update TypeUtils in typeutils.cpp

b4e70da

Use IsSpmiTarget64Bit() in compileresult.cpp

74681c6

Replace sizeof(void*) -> SpmiTargetPointerSize() in methodcontext.cpp

3ad3238

echesakov requested a review from BruceForstall February 26, 2021 04:20

BruceForstall approved these changes Feb 26, 2021

View reviewed changes

echesakov added 2 commits February 26, 2021 10:56

Fix warning in methodcontext.cpp

d280c5b

Nit: "theor" -> "their" in neardiffer.cpp

5f9e9fd

echesakov added the auto-merge label Feb 26, 2021

echesakov merged commit b767622 into dotnet:master Feb 26, 2021

echesakov deleted the SuperPMI-Arm64 branch February 26, 2021 22:40

echesakov mentioned this pull request Mar 8, 2021

Failures in arm64 superpmi asm diffs dotnet/jitutils#309

Closed

ghost locked as resolved and limited conversation to collaborators Mar 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperPMI] Support spmi-asmdiffs on Arm64 #48683

[SuperPMI] Support spmi-asmdiffs on Arm64 #48683

echesakov commented Feb 24, 2021 •

edited

Loading

echesakov Feb 25, 2021

echesakov Feb 26, 2021

BruceForstall Feb 26, 2021

echesakov commented Feb 25, 2021

BruceForstall left a comment

BruceForstall left a comment

BruceForstall Feb 26, 2021

ghost commented Feb 26, 2021

echesakov commented Feb 26, 2021

[SuperPMI] Support spmi-asmdiffs on Arm64 #48683

[SuperPMI] Support spmi-asmdiffs on Arm64 #48683

Conversation

echesakov commented Feb 24, 2021 • edited Loading

echesakov Feb 25, 2021

Choose a reason for hiding this comment

echesakov Feb 26, 2021

Choose a reason for hiding this comment

BruceForstall Feb 26, 2021

Choose a reason for hiding this comment

echesakov commented Feb 25, 2021

BruceForstall left a comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

BruceForstall Feb 26, 2021

Choose a reason for hiding this comment

ghost commented Feb 26, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

echesakov commented Feb 26, 2021

echesakov commented Feb 24, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.