Fix arm64 prolog generation for register masks with holes. by sandreenko · Pull Request #21395 · dotnet/coreclr

sandreenko · 2018-12-06T02:05:37Z

This PR cleans genSaveCalleeSavedRegistersHelp and genRestoreCalleeSavedRegistersHelp to delete code duplication and then fixes issue #21363.

No diffs on arm64 checked (-c, -f) with crossgen and pmi.

644977e: Check that genSaveCalleeSavedRegistersHelp doesn't accept REG_LR.

There are two callers and both save LR with FP before calling this helper.
In case if somebody calls this function wthREG_LR it won't work because unwinding info doesn't expect holes in the reg pairs.

7a51619: Extract genPushOrPopCalleeSavedRegisters.

It will be used for both float and int types.

11c4e0d: Extend genSaveCaleeSavedRegisterGroup to support float.

The previous version was a copy-paste with an additional case for REG_LR that was unreachable. Fix that.

ac622c1: Use genSaveCaleeSavedRegisterGroup for floats.

fa0be31: Extract genRestoreCaleeSavedRegisterGroup.

It will be used for both int and float groups.

f508521: Prepare genPopCalleeSavedRegisters to work with float.

3e92518: Use genRestoreCaleeSavedRegisterGroup for float.

c088641: Check that genRestoreCalleeSavedRegistersHelp doesn't restore REG_LR.

Both callers do it later.

e36ab6d: Extract CheckSPOffset.

and move it out of loops because it works only before the first save intruction.

d9fd6ff: Format genRestoreCaleeSavedRegisterGroup as genSaveCalleeSavedRegistersHelp.

6870f6f: Extract buildRegPairsStack from genSaveCaleeSavedRegisterGroup and genRestoreCaleeSavedRegisterGroup.

Build a stack of registers that we want to save/restore and then iterate over it doing the actual saving/restoring.

ec076a1: Extract GetSlotSizeForRegsInMask.

6f0fa31: Tolerate holes in arm64 prolog/epilog register masks.

Fixes #21363

b91d19f: Reenable the test.

90c355b: Add new methods headers.

sandreenko · 2018-12-06T21:15:53Z

As bonus I will improve our prolog/epilog generation by using save_next. It is clear how to do it after the cleaning.

It will give us few nice asm diffs on arm64.

sandreenko · 2018-12-06T21:17:10Z

PTAL @BruceForstall @dotnet/jit-contrib

sandreenko · 2018-12-07T01:22:50Z

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs0x10 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs2 Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs0x1000 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked pmi_asm_diffs
@dotnet-bot test Windows_NT x64_arm64_altjit Checked pmi_asm_diffs

sandreenko · 2018-12-07T03:17:22Z

arm64 pmi fails with

error NU1605: Detected package downgrade: System.Collections from 4.3.0 to 4.0.11. Reference the package directly from the project to select a different version. [/home/robox/j/workspace/dotnet_coreclr/master/arm64_cross_checked_ubuntu16.04_pmi_asm_diffs_tst_prtest/_/pmi/jitutils/jitutils.sln]

Is it a known issue?

BruceForstall · 2018-12-07T07:17:57Z

Not known by me.

Looks like an issue building jitutils on arm64.

sandreenko · 2018-12-07T23:37:57Z

@dotnet-bot test
Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs2 Build and Test
@dotnet-bot Windows_NT arm64 Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs0x1000 Build and Test

sandreenko · 2018-12-07T23:38:14Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests

sandreenko · 2018-12-07T23:48:47Z

@dotnet-bot Windows_NT arm64 Cross Checked Innerloop Build and Test

sandreenko · 2018-12-12T01:03:21Z

@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs0x1000 Build and Test

sandreenko · 2018-12-12T01:03:50Z

Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs2 Build and Test fails due to #21500 .

sandreenko · 2018-12-14T00:06:57Z

@BruceForstall Could you please take another look?

BruceForstall

A few minor comments.

As we discussed in person, I'm mostly concerned about testing. So:

There should be no asm diffs with this change (including unwind codes) which you indicated is true.
You should attempt to verify that unwinding with the newly generated unwind info that includes "gaps" should work. My suggestion was to add a register stress mode that causes there to be many gaps, like removing all the odd callee-saved registers from the set of available registers. Then, you need to force unwinding to happen. One way is to run GCStress=4/8/C.

Overall, looks like a nice simplification and cleanup to the code.

BruceForstall · 2018-12-18T00:25:58Z

+//   spOffset       - stack pointer offset value;
+//   slotSize       - stack slot size in bytes.
+//
+void CheckSPOffset(bool isRegsCountOdd, int spOffset, int slotSize)


I'm not fond of this and the functions below (buildRegPairsStack, GetSlotSizeForRegsInMask) being unprototyped, file-level functions. I'd prefer they be members of CodeGen, perhaps static members.

We do not have a separate header file like codegenarm64.h, so I will have to place it into codegen.h under #if defined(_TARGET_ARM64_) && defined(DEBUG), do you think it is worth it?

Yes, please. (The others don't need DEBUG)

Fixed in b889dc2 , does it look better?

sandreenko · 2018-12-20T22:07:02Z

You should attempt to verify that unwinding with the newly generated unwind info that includes "gaps" should work. My suggestion was to add a register stress mode that causes there to be many gaps, like removing all the odd callee-saved registers from the set of available registers. Then, you need to force unwinding to happen. One way is to run GCStress=4/8/C.

Checked, got clean run with gcStress=0xc and 50% holes.

Please review it again.

There are two callers and both save `LR` with `FP` before calling this helper. In case if somebody calls this function wth`REG_LR` it won't work because unwinding info doesn't expect holes in the reg pairs.

It will be used for both float and int types.

The previous version was a copy-paste with an additional case for `REG_LR` that was unreachable. Fix that.

It will be used for both int and float groups.

…LR`. Both callers do it later.

and move it out of loops because it works only before the first save intruction.

…stersHelp`.

…d `genRestoreCaleeSavedRegisterGroup`. Build a stack of registers that we want to save/restore and then iterate over it doing the actual saving/restoring.

Fixes #21363

…ucts.

For future use.

sandreenko · 2018-12-21T00:54:21Z

@BruceForstall The PR was updated,please take another look.

BruceForstall

LGTM if tests pass. Please trigger some arm64 jobs.

sandreenko · 2018-12-21T01:07:07Z

@dotnet-bot test
Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs2 Build and Test
@dotnet-bot Windows_NT arm64 Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs0x1000 Build and Test

sandreenko · 2018-12-21T18:23:15Z

test Windows_NT arm Cross Debug Innerloop Build
test Windows_NT arm64 Cross Debug Innerloop Build
test Windows_NT arm Cross Checked Innerloop Build and Test

…reclr#21395) * Check that `genSaveCalleeSavedRegistersHelp` doesn't accept `REG_LR`. There are two callers and both save `LR` with `FP` before calling this helper. In case if somebody calls this function wth`REG_LR` it won't work because unwinding info doesn't expect holes in the reg pairs. * Extract `genPushOrPopCalleeSavedRegisters`. It will be used for both float and int types. * Extend `genSaveCaleeSavedRegisterGroup` to support float. The previous version was a copy-paste with an additional case for `REG_LR` that was unreachable. Fix that. * Use `genSaveCaleeSavedRegisterGroup` for floats. * Extract `genRestoreCaleeSavedRegisterGroup`. It will be used for both int and float groups. * Prepare `genPopCalleeSavedRegisters` to work with float. * Use `genRestoreCaleeSavedRegisterGroup` for float. * Check that `genRestoreCalleeSavedRegistersHelp` doesn't restore `REG_LR`. Both callers do it later. * Extract `CheckSPOffset`. and move it out of loops because it works only before the first save intruction. * Format `genRestoreCaleeSavedRegisterGroup` as `genSaveCalleeSavedRegistersHelp`. * Extract `buildRegPairsStack` from `genSaveCaleeSavedRegisterGroup` and `genRestoreCaleeSavedRegisterGroup`. Build a stack of registers that we want to save/restore and then iterate over it doing the actual saving/restoring. * Extract `GetSlotSizeForRegsInMask`. * Tolerate holes in arm64 prolog/epilog register masks. Fixes dotnet/coreclr#21363 * Reenable the test. * Add new methods headers. * Fix typos. * Do not use non-const references. * Describe `buildRegPairsStack` better. * Change signature of `buildRegPairsStack` to avoid copyings of big structs. * Return the logic for `RBM_LR`. For future use. * Clean-up some unused variables. * Fix comments. * Get rid of file-level functions. * Make new methods static. Commit migrated from dotnet/coreclr@a9ceb15

sandreenko added area-CodeGen arch-arm64 labels Dec 6, 2018

sandreenko force-pushed the FixArm64PrologGeneration branch from 5376715 to 90c355b Compare December 6, 2018 21:11

BruceForstall suggested changes Dec 6, 2018

View reviewed changes

Comment thread src/jit/codegen.h Outdated

Comment thread src/jit/codegenarm64.cpp Outdated

Comment thread src/jit/codegenarm64.cpp Outdated

Comment thread src/jit/codegenarm64.cpp Outdated

Comment thread src/jit/codegenarm64.cpp Outdated

Comment thread src/jit/codegenarm64.cpp Outdated

BruceForstall previously requested changes Dec 18, 2018

View reviewed changes

sandreenko force-pushed the FixArm64PrologGeneration branch from a677a64 to b889dc2 Compare December 20, 2018 22:05

sandreenko mentioned this pull request Dec 21, 2018

Fix ArrayStack's call to default constructor for <T>. #21624

Merged

Sergey Andreenko added 10 commits December 20, 2018 16:33

Check that genSaveCalleeSavedRegistersHelp doesn't accept REG_LR.

60b6775

There are two callers and both save `LR` with `FP` before calling this helper. In case if somebody calls this function wth`REG_LR` it won't work because unwinding info doesn't expect holes in the reg pairs.

Extract genPushOrPopCalleeSavedRegisters.

29805f0

It will be used for both float and int types.

Extend genSaveCaleeSavedRegisterGroup to support float.

7252726

The previous version was a copy-paste with an additional case for `REG_LR` that was unreachable. Fix that.

Use genSaveCaleeSavedRegisterGroup for floats.

43c7ba9

Extract genRestoreCaleeSavedRegisterGroup.

9adcfe8

It will be used for both int and float groups.

Prepare genPopCalleeSavedRegisters to work with float.

a1a2549

Use genRestoreCaleeSavedRegisterGroup for float.

3d85f13

Check that genRestoreCalleeSavedRegistersHelp doesn't restore `REG_…

10d2e72

…LR`. Both callers do it later.

Extract CheckSPOffset.

8c8fba2

and move it out of loops because it works only before the first save intruction.

Format genRestoreCaleeSavedRegisterGroup as `genSaveCalleeSavedRegi…

667f2ef

…stersHelp`.

Sergey Andreenko added 13 commits December 20, 2018 16:33

Extract buildRegPairsStack from genSaveCaleeSavedRegisterGroup an…

15adca7

…d `genRestoreCaleeSavedRegisterGroup`. Build a stack of registers that we want to save/restore and then iterate over it doing the actual saving/restoring.

Extract GetSlotSizeForRegsInMask.

08033a7

Tolerate holes in arm64 prolog/epilog register masks.

e606e78

Fixes #21363

Reenable the test.

5a64b77

Add new methods headers.

66731e4

Fix typos.

9bd9cb5

Do not use non-const references.

24b6813

Describe buildRegPairsStack better.

886d9af

Change signature of buildRegPairsStack to avoid copyings of big str…

833cba8

…ucts.

Return the logic for RBM_LR.

ae42682

For future use.

Clean-up some unused variables.

ecdefc6

Fix comments.

477023c

Get rid of file-level functions.

46010a5

sandreenko force-pushed the FixArm64PrologGeneration branch from b889dc2 to 46010a5 Compare December 21, 2018 00:34

Make new methods static.

178c0db

BruceForstall approved these changes Dec 21, 2018

View reviewed changes

sandreenko merged commit a9ceb15 into dotnet:master Dec 21, 2018

sandreenko mentioned this pull request Dec 27, 2018

Use saveNext unwind opcode on arm64. #21683

Merged

sandreenko deleted the FixArm64PrologGeneration branch January 23, 2019 00:12

Conversation

sandreenko commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandreenko commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandreenko commented Dec 6, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandreenko commented Dec 7, 2018

Uh oh!

sandreenko commented Dec 7, 2018

Uh oh!

BruceForstall commented Dec 7, 2018

Uh oh!

sandreenko commented Dec 7, 2018

Uh oh!

sandreenko commented Dec 7, 2018

Uh oh!

sandreenko commented Dec 7, 2018

Uh oh!

sandreenko commented Dec 12, 2018

Uh oh!

sandreenko commented Dec 12, 2018

Uh oh!

sandreenko commented Dec 14, 2018

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BruceForstall Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

sandreenko Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

BruceForstall Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

sandreenko Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandreenko commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandreenko commented Dec 21, 2018

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

sandreenko commented Dec 21, 2018

Uh oh!

sandreenko commented Dec 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sandreenko commented Dec 6, 2018 •

edited

Loading

sandreenko commented Dec 6, 2018 •

edited

Loading

sandreenko commented Dec 20, 2018 •

edited

Loading