Do not fast tail call if caller has multislot structs by jashook · Pull Request #25885 · dotnet/coreclr

jashook · 2019-07-25T17:59:51Z

With more eyes on LowerFastTailCall, @jakobbotsch and I have found another SBC bug. We have verified that this causes silent bad codegen on <= coreclr 3.0. Thank you @jakobbotsch for help on a functioning repro.

jashook · 2019-07-25T18:00:07Z

Note this effects unix x64 only.

jashook · 2019-07-25T18:05:16Z

/cc @RussKeldorph

Note I discovered that this change should be in #20643 and originally added it there. Then realized this is most likely a bug going back. So this change has been tested already. It is just waiting on reviews. Will hoist it out of #20643.

jashook · 2019-07-25T18:06:24Z

Fixes #25884

jashook · 2019-07-25T18:17:49Z

/cc @dotnet/jit-contrib

AndyAyersMS

Do we see any diffs from this?

jashook · 2019-07-25T20:02:11Z

I would argue that this change does not need to run diffs. compHasMultiSlotArgs and varDscInfo->hasMultiSlotStruct are only used in fastTailCall decisions, specifically on unix. So any diff would be a new correct decision not to fast tail call. Regardless we can be almost entirely confident that this fastTailCall decision is not currently being made in our test suite.

That being said, this should be a very quick run and a very easy zero diff example. Will report back.

AndyAyersMS · 2019-07-25T20:48:45Z

I would argue that this change does not need to run diffs.

Running diffs should be cheap enough now that we should always do it as a matter of practice.

I am curious about a few things:

If we think we should back port this fix into 2.x, we should check if this bug causes bad code in our frameworks, and if there is, why the testing we already do hasn't uncovered this.
We might realize this check is overly conservative and blocks tails calls that are perfectly safe. If so we should add a work item to make sure those cases get handled once we've fixed the analysis to understand these types of args.

CarolEidt

LGTM with a couple of comment changes (mentioned offline)

jashook · 2019-07-26T22:48:59Z

@AndyAyersMS there are diffs with this PR, they effect the SIMD tests. Unfortunately superpmi has a bug in exception handling so the actually diffs from jit-analyze are not interesting. I will investigate further on Monday.

On manual inspection we now will not fast tail call for SIMD types more often. This happens infrequently, but will be fixed with the work @jakobbotsch is doing in LowerFastTail call. In the meantime, it is more important to be correct then continue to fast tail call.

jashook · 2019-07-26T23:19:17Z

Found 872 files with textual diffs.

Summary:
(Lower is better)

Total bytes of diff: -142162 (-98.893% of base)
    diff is an improvement.

Top file improvements by size (bytes):
        -231 : 218124.dasm (-100.000% of base)
        -231 : 218190.dasm (-100.000% of base)
        -231 : 218256.dasm (-100.000% of base)
        -231 : 131457.dasm (-100.000% of base)
        -231 : 131525.dasm (-100.000% of base)

872 total files with size differences (872 improved, 0 regressed), 9 unchanged.

Top method improvements by size (bytes):
        -231 (-100.000% of base) : 218124.dasm - JIT.HardwareIntrinsics.X86.SimpleTernaryOpTest__MultiplyAddSingle:ValidateResult(struct,struct,struct,long,ref):this
        -231 (-100.000% of base) : 218190.dasm - JIT.HardwareIntrinsics.X86.SimpleTernaryOpTest__MultiplyAddNegatedSingle:ValidateResult(struct,struct,struct,long,ref):this
        -231 (-100.000% of base) : 218256.dasm - JIT.HardwareIntrinsics.X86.AlternatingTernaryOpTest__MultiplyAddSubtractSingle:ValidateResult(struct,struct,struct,long,ref):this
        -231 (-100.000% of base) : 131457.dasm - JIT.HardwareIntrinsics.X86.SimpleTernaryOpTest__BlendVariableInt16:ValidateResult(struct,struct,struct,long,ref):this
        -231 (-100.000% of base) : 131525.dasm - JIT.HardwareIntrinsics.X86.SimpleTernaryOpTest__BlendVariableInt32:ValidateResult(struct,struct,struct,long,ref):this

Top method improvements by size (percentage):
        -217 (-100.000% of base) : 197444.dasm - JIT.HardwareIntrinsics.X86.AlternatingTernaryOpTest__MultiplyAddSubtractSingle:ValidateResult(struct,struct,struct,long,ref):this
        -176 (-100.000% of base) : 116303.dasm - JIT.HardwareIntrinsics.X86.SimpleBinaryOpTest__AndByte:ValidateResult(struct,struct,long,ref):this
        -176 (-100.000% of base) : 11990.dasm - JIT.HardwareIntrinsics.X86.SimpleBinaryOpTest__XorSingle:ValidateResult(struct,struct,long,ref):this
        -176 (-100.000% of base) : 117007.dasm - JIT.HardwareIntrinsics.X86.SimpleBinaryOpTest__CompareEqualInt32:ValidateResult(struct,struct,long,ref):this
        -127 (-100.000% of base) : 118521.dasm - JIT.HardwareIntrinsics.X86.SimpleUnaryOpConvTest__ConvertToVector128DoubleVector128Single:ValidateResult(struct,long,ref):this

872 total methods with size differences (872 improved, 0 regressed), 9 unchanged.

Diffs, note the percentages are off due to an issue with PAL_TRY exception filtering. The is run over a superpmi diff of a superpmi collection of the test assets.

jashook · 2019-07-26T23:29:49Z

Change has been previously validated. Merging.

* Do not fast tail call if caller has multislot structs * apply patch * Address offline feedback

* Use fgInitArgInfo in fgCanFastTailCall This leverages the work done in #19658 to use fgInitArgInfo in fgCanFastTailCall. This removes much of the duplicate ABI code that was required before #19658. In addition, the change more clearly explains the issues with LowerFastTailCall and more clearly bails on when it can and cannot fastTailCall. Note: Part of the old logic would report no to a fastTailCall on Windows AMD64, if the struct was a power of two (engregisterable). This has been removed; however, it is unclear currently whether LowerFastTailCall/Codgen will support the change. * Address pr feedback for FastTailCallCandidates * Update variable name based on rebase * Fix comment headers based on feedback * More feedback addressed * Fix rebase error * Address feedback and correctly bail on byref args * Restrict to only bail on byref struct args * Correctly used computations from fgArgInfo * Correctly re-init arg infor for explicit tailcalls This is required because morphTailCall modifies the argument list. It would be nice in the future to go through each argument on subsequent calls to fgInitArgInfo to verify there have been no changes. * Fix x86 assert * Address PR feedback related to recomputing initArgs * Fix comment * Update fgCanFastTailCall header * Address PR feedback * Apply format patch * Correctly reset arginfo after morphtailcall * Commit does several small things, and fixes a bug Addresses PR feedback Removes dead code Changes to bail a fast tail call when the caller has a multi slot struct * Fix windows build * Address pr feedback * Fix arm and x86 builds * apply format patch and remove #25885 * Rebase on master * Apply format patch

…#25885) * Do not fast tail call if caller has multislot structs * apply patch * Address offline feedback Commit migrated from dotnet/coreclr@059604d

* Use fgInitArgInfo in fgCanFastTailCall This leverages the work done in dotnet/coreclr#19658 to use fgInitArgInfo in fgCanFastTailCall. This removes much of the duplicate ABI code that was required before dotnet/coreclr#19658. In addition, the change more clearly explains the issues with LowerFastTailCall and more clearly bails on when it can and cannot fastTailCall. Note: Part of the old logic would report no to a fastTailCall on Windows AMD64, if the struct was a power of two (engregisterable). This has been removed; however, it is unclear currently whether LowerFastTailCall/Codgen will support the change. * Address pr feedback for FastTailCallCandidates * Update variable name based on rebase * Fix comment headers based on feedback * More feedback addressed * Fix rebase error * Address feedback and correctly bail on byref args * Restrict to only bail on byref struct args * Correctly used computations from fgArgInfo * Correctly re-init arg infor for explicit tailcalls This is required because morphTailCall modifies the argument list. It would be nice in the future to go through each argument on subsequent calls to fgInitArgInfo to verify there have been no changes. * Fix x86 assert * Address PR feedback related to recomputing initArgs * Fix comment * Update fgCanFastTailCall header * Address PR feedback * Apply format patch * Correctly reset arginfo after morphtailcall * Commit does several small things, and fixes a bug Addresses PR feedback Removes dead code Changes to bail a fast tail call when the caller has a multi slot struct * Fix windows build * Address pr feedback * Fix arm and x86 builds * apply format patch and remove dotnet/coreclr#25885 * Rebase on master * Apply format patch Commit migrated from dotnet/coreclr@f8f0c55

Do not fast tail call if caller has multislot structs

9b19651

jashook requested review from BruceForstall, CarolEidt and jakobbotsch July 25, 2019 17:59

jakobbotsch approved these changes Jul 25, 2019

View reviewed changes

apply patch

d86f929

jashook pushed a commit to jashook/coreclr that referenced this pull request Jul 25, 2019

apply format patch and remove dotnet#25885

f87651e

jashook requested a review from AndyAyersMS July 25, 2019 18:22

AndyAyersMS approved these changes Jul 25, 2019

View reviewed changes

jashook mentioned this pull request Jul 25, 2019

Update fgCanFastTailCall to use new fgArgInfo #20643

Merged

jakobbotsch approved these changes Jul 25, 2019

View reviewed changes

CarolEidt approved these changes Jul 25, 2019

View reviewed changes

jashook added 2 commits July 26, 2019 13:59

Address no diffs

ce64fec

Pessimize caller fast tail calls

0815a63

Address offline feedback

cf41991

jashook merged commit 059604d into dotnet:master Jul 26, 2019

jashook deleted the mitigate_gh_25884 branch July 26, 2019 23:30

jashook pushed a commit to jashook/coreclr that referenced this pull request Jul 26, 2019

Do not fast tail call if caller has multislot structs (dotnet#25885)

d938b66

* Do not fast tail call if caller has multislot structs * apply patch * Address offline feedback

jashook pushed a commit to jashook/coreclr that referenced this pull request Jul 26, 2019

Do not fast tail call if caller has multislot structs (dotnet#25885)

ee104e0

* Do not fast tail call if caller has multislot structs * apply patch * Address offline feedback

jashook pushed a commit to jashook/coreclr that referenced this pull request Jul 27, 2019

apply format patch and remove dotnet#25885

f7b0d28

jashook mentioned this pull request Jul 28, 2019

Do not fast tail call if caller has multislot structs (#25885) #25910

Merged

jashook pushed a commit that referenced this pull request Jul 28, 2019

Do not fast tail call if caller has multislot structs (#25885) (#25910)

ca3c39c

* Do not fast tail call if caller has multislot structs * apply patch * Address offline feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not fast tail call if caller has multislot structs#25885

Do not fast tail call if caller has multislot structs#25885
jashook merged 5 commits intodotnet:masterfrom
jashook:mitigate_gh_25884

jashook commented Jul 25, 2019

Uh oh!

jashook commented Jul 25, 2019

Uh oh!

jashook commented Jul 25, 2019 •

edited

Loading

Uh oh!

jashook commented Jul 25, 2019 •

edited

Loading

Uh oh!

jashook commented Jul 25, 2019

Uh oh!

AndyAyersMS left a comment

Uh oh!

jashook commented Jul 25, 2019 •

edited

Loading

Uh oh!

AndyAyersMS commented Jul 25, 2019

Uh oh!

CarolEidt left a comment

Uh oh!

jashook commented Jul 26, 2019

Uh oh!

jashook commented Jul 26, 2019 •

edited

Loading

Uh oh!

jashook commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jashook commented Jul 25, 2019

Uh oh!

jashook commented Jul 25, 2019

Uh oh!

jashook commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jashook commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jashook commented Jul 25, 2019

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

jashook commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Jul 25, 2019

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

jashook commented Jul 26, 2019

Uh oh!

jashook commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jashook commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jashook commented Jul 25, 2019 •

edited

Loading

jashook commented Jul 25, 2019 •

edited

Loading

jashook commented Jul 25, 2019 •

edited

Loading

jashook commented Jul 26, 2019 •

edited

Loading