Simplify handling of multi-reg returns by SingleAccretion · Pull Request #74499 · dotnet/runtime

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

SingleAccretion · 2022-08-24T12:50:40Z

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes#36868.

Tiny amount of diffs on ARM64: some forward substitutions of non-multireg SIMDs are now allowed.

ghost · 2022-08-24T12:50:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Keep the return descriptor for the method being compiled and use it in various places.

Deletes the dependence of multi-reg RETURN backend code on precise handles of LCL_VAR operands.

Fixes #36868.

Depends on #72887.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

SingleAccretion · 2022-09-08T15:45:30Z

@dotnet/jit-contrib

AndyAyersMS · 2022-09-16T18:21:02Z

@dotnet/jit-contrib anyone up for reviewing this? Looks like a nice cleanup but cuts across a lot of code.

jakobbotsch · 2022-09-22T13:50:09Z

I can take a look, but it'll probably take me some time to get familiar with multi-reg returns.

Add the return type descriptor to compiler and use it. Delete target-specific code from "impFixupStructReturnType". The main target of this change is getting rid of the dependency of multi-reg RETURN backend code on exact struct handles on temps it expects as the sources.

Due to the buggy nature of the old code, the matrix for what substititions were allowed was as follows: ARM x86 LONG not-LCL_VAR NONE STRUCT LCL_VAR LCL_VAR This commit preserves this extremely quirky behavior. Unfortunately, just enabling the propagation is a CQ regression due to some RA issues.

jakobbotsch · 2022-10-04T20:03:34Z

+ // TODO-Review: this seems unnecessary. Return ABI doesn't change under varargs.
+ && !op->AsCall()->IsVarargs()
+#endif // defined(TARGET_ARMARCH) || defined(TARGET_LOONGARCH64)
+ )


Seems likely this check is not necessary at all anymore after #73059.

Ah, not really.

jakobbotsch

This looks great to me, very nice cleanup. I'll trigger some stress legs.

jakobbotsch · 2022-10-04T20:09:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-04T20:09:48Z

Azure Pipelines successfully started running 1 pipeline(s).

SingleAccretion · 2022-10-06T18:10:58Z

Stress failures look like #76550, #76507 and #76280.

jakobbotsch · 2022-10-06T20:38:30Z

Thanks!

ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Aug 24, 2022

SingleAccretion force-pushed the Return-Cleanup branch from 20bd20a to 85abb89Compare August 24, 2022 13:44

This was referenced Aug 24, 2022
Infra improvements for Helix #68176
Closed
Methodical_others test JIT/Methodical/Coverage/copy_prop_byref_to_native_int crashing #69832
Open
Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040
Closed

SingleAccretion force-pushed the Return-Cleanup branch from 85abb89 to f46440eCompare August 24, 2022 18:09

SingleAccretion force-pushed the Return-Cleanup branch 2 times, most recently from 45bd7f9 to a5828ebCompare September 7, 2022 20:57

SingleAccretion closed this Sep 8, 2022

SingleAccretion reopened this Sep 8, 2022

SingleAccretion marked this pull request as ready for review September 8, 2022 15:44

jakobbotsch self-assigned this Sep 22, 2022

jakobbotsch self-requested a review September 22, 2022 13:50

SingleAccretion added 2 commits September 25, 2022 22:51

SingleAccretion force-pushed the Return-Cleanup branch from 030d67c to e4a948eCompare September 25, 2022 19:53

SingleAccretion mentioned this pull request Oct 2, 2022
NativeAOT-LLVM: Turn on optimisation in RyuJit module, partial support for FIELD_LIST and lowering promoted fields dotnet/runtimelab#2007
Merged

jakobbotsch reviewed Oct 4, 2022
View reviewed changes

jakobbotsch approved these changes Oct 4, 2022
View reviewed changes

jakobbotsch merged commit be5a4d3 into dotnet:mainOct 6, 2022

SingleAccretion deleted the Return-Cleanup branch October 6, 2022 23:17

ghost locked as resolved and limited conversation to collaborators Nov 6, 2022