Disable matching constants for vectors that needs upper half to be save/restore #74110

kunalspathak · 2022-08-17T23:17:44Z

~~This reverts commit 24f5de4.~~

#70171 reuses Vector constants in register allocation but LSRA doesn't handle it very well. It doesn't have a semantics of looking into constant registers and save/restore the upper-half of them during the call. Because of this, the constant register that we want to reuse after the call gets garbage data in the upper-half and leads to silent functional differences.

There were several alternatives tried:

During buildUpperVectorSave() also check if there are any vector registers holding constants and add RefPosition to save/restore, but we don't have that information until the register allocation.
Piggyback on one of the RefTypeKill refposition and clear the constant vector registers mask during allocation, but we don't have the treeNode information for RefTypeKill to accurately do this only for Calls and not for anything else.

~~For now, I am reverting the change and handle it in better way as part of #70182.~~

For now, I am disabling the IsMatchingConstant() optimization for registers whose upper half need save/restore.

Fixes: #70642

…nstant (dotnet#70171)" This reverts commit 24f5de4.

ghost · 2022-08-17T23:17:58Z

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

Issue Details

This reverts commit 24f5de4.

#70171 reuses Vector constants in register allocation but LSRA doesn't handle it very well. It doesn't have a semantics of looking into constant registers and save/restore the upper-half of them during the call. Because of this, the constant register that we want to reuse after the call gets garbage data in the upper-half and leads to silent functional differences.

There were several alternatives tried:

During buildUpperVectorSave() also check if there are any vector registers holding constants and add RefPosition to save/restore, but we don't have that information until the register allocation.
Piggyback on one of the RefTypeKill refposition and clear the constant vector registers mask during allocation, but we don't have the treeNode information for RefTypeKill to accurately do this only for Calls and not for anything else.

For now, I am reverting the change and handle it in better way as part of #70182.

Fixes: #70642

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

kunalspathak · 2022-08-17T23:17:59Z

@tannergooding @BruceForstall @dotnet/jit-contrib

tannergooding · 2022-08-18T00:03:47Z

src/coreclr/jit/lsra.cpp

-        case GT_CNS_VEC:
-        {
-            return GenTreeVecCon::Equals(refPosition->treeNode->AsVecCon(), otherTreeNode->AsVecCon());
-        }


Instead of reverting, would it be feasible for this to be the following?

return (otherTreeNode->gtType != TYP_SIMD32) && GenTreeVecCon::Equals(refPosition->treeNode->AsVecCon(), otherTreeNode->AsVecCon());

Asking as the original change had some measured perf wins and since it looks like the bug only impacts TYP_SIMD32 due to the need to save/restore the upper halves (meaning I think it shouldn't impact TYP_SIMD8/12/16).

If we want to scope it, we only need to do it for platforms where FEATURE_PARTIAL_SIMD_CALLEE_SAVE is defined (win-amd64, arm64, LoongArch64). Presumably there is no issue on x86?

varTypeNeedsPartialCalleeSave would need to be used.

However, I still think it's probably "too late" to construct a fix to ensure correctness, on all platforms, when it's trivial to revert the change.

varTypeNeedsPartialCalleeSave check seems reasonable to include, but I agree with @BruceForstall that the simplest thing would be to revert the change at this point.

👍, just thought I'd throw it out as a suggestion given this will have negative impact on Arm64.

I expect we'll see this one dotnet/perf-autofiling-issues#5976 and a few of the other code paths that use vector constants regress a bit.

I looked at the spmi-diffs and there are few in benchmarks. I tried the change to just use varTypeNeedsPartialCalleeSave and didn't see as many spmi-diffs in benchmarks, asp locally. I will let CI run and check other diffs.

There is no change in spmi diff on Arm64 if we just disable isMatchingConstant for varTypeNeedsPartialCalleeSave. On x64, there is slight difference (earlier in benchmark collection, 12 methods regressed and now 5).

it looks like the bug only impacts TYP_SIMD32 due to the need to save/restore the upper halves (meaning I think it shouldn't impact TYP_SIMD8/12/16).

@tannergooding , even Arm64 needs save/restore upper half as per this comment, so it could be problematic there too, right?

runtime/src/coreclr/jit/compiler.h

Lines 7439 to 7445 in 519d1e3

static bool varTypeNeedsPartialCalleeSave(var_types type)

{

assert(type != TYP_STRUCT);

// ARM64 ABI FP Callee save registers only require Callee to save lower 8 Bytes

// For SIMD types longer than 8 bytes Caller is responsible for saving and restoring Upper bytes.

return ((type == TYP_SIMD16) || (type == TYP_SIMD12));

}

Ah, so it is (https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#simd-and-floating-point-registers):

Registers v8-v15 must be preserved by a callee across subroutine calls; the remaining registers (v0-v7, v16-v31) do not need to be preserved (or should be preserved by the caller). Additionally, only the bottom 64 bits of each value stored in v8-v15 need to be preserved [7]; it is the responsibility of the caller to preserve larger values.

I thought the entire register needed to be saved, but apparently not, that's unfortunate.

There is no change in spmi diff on Arm64 if we just disable isMatchingConstant for varTypeNeedsPartialCalleeSave. On x64, there is slight difference (earlier in benchmark collection, 12 methods regressed and now 5).

What was the regression on Arm64 if we revert the entire PR?

Getting the regression down to just 5 on x64 sounds 👍

What was the regression on Arm64 if we revert the entire PR?

There is no change between total revert and limiting it to just varTypeNeedsPartialCalleeSave on Arm64.

https://dev.azure.com/dnceng/public/_build/results?buildId=1951028&view=ms.vss-build-web.run-extensions-tab

(https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#simd-and-floating-point-registers):

Thanks for this incredible resource. I didn't know about it.

…tchingConstant (dotnet#70171)"" This reverts commit 984120f.

JulieLeeMSFT · 2022-08-18T17:47:46Z

CC @jeffschwMSFT, this is the bug fix for #70642, and we are reverting it.

jeffschwMSFT · 2022-08-18T17:51:05Z

If wanted in 7, we need to backport to 7 RC1

BruceForstall

LGTM

Please make sure you change the title and text of this PR to reflect the current change (it's no longer "revert". Same for the commit message when you merge.

kunalspathak · 2022-08-18T22:31:10Z

/backport to release/7.0-rc1

github-actions · 2022-08-18T22:31:22Z

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2885813879

kunalspathak · 2022-08-18T23:22:15Z

Trigged a run https://dev.azure.com/dnceng/public/_build/results?buildId=1952719&view=results

kunalspathak · 2022-08-19T15:01:45Z

The test no longer fails in https://dev.azure.com/dnceng/public/_build/results?buildId=1952719&view=results .

Revert "Ensure that GT_CNS_VEC is handled in LinearScan::isMatchingCo…

984120f

…nstant (dotnet#70171)" This reverts commit 24f5de4.

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

ghost assigned kunalspathak Aug 17, 2022

BruceForstall approved these changes Aug 17, 2022

View reviewed changes

BruceForstall added this to the 7.0.0 milestone Aug 17, 2022

tannergooding reviewed Aug 18, 2022

View reviewed changes

kunalspathak added 3 commits August 17, 2022 23:01

Revert "Revert "Ensure that GT_CNS_VEC is handled in LinearScan::isMa…

6f65b59

…tchingConstant (dotnet#70171)"" This reverts commit 984120f.

Add partial save check

bc07709

Use the correct register type

56cb006

BruceForstall approved these changes Aug 18, 2022

View reviewed changes

kunalspathak changed the title ~~Revert #70171 "Ensure that GT_CNS_VEC is handled in ..."~~ Disable matching constants for vectors that needs upper half to be save/restore Aug 18, 2022

kunalspathak merged commit 64d27bb into dotnet:main Aug 18, 2022

github-actions bot mentioned this pull request Aug 18, 2022

[release/7.0-rc1] Disable matching constants for vectors that needs upper half to be save/restore #74192

Merged

kunalspathak deleted the revert-vec-con branch August 18, 2022 23:22

kunalspathak mentioned this pull request Aug 20, 2022

Spmi-diff is showing diffs that are not related to the PR #74287

Closed

ghost locked as resolved and limited conversation to collaborators Sep 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable matching constants for vectors that needs upper half to be save/restore #74110

Disable matching constants for vectors that needs upper half to be save/restore #74110

kunalspathak commented Aug 17, 2022 •

edited

Loading

ghost commented Aug 17, 2022

kunalspathak commented Aug 17, 2022

tannergooding Aug 18, 2022

BruceForstall Aug 18, 2022

BruceForstall Aug 18, 2022 •

edited

Loading

kunalspathak Aug 18, 2022

tannergooding Aug 18, 2022

kunalspathak Aug 18, 2022

kunalspathak Aug 18, 2022

tannergooding Aug 18, 2022

kunalspathak Aug 18, 2022

kunalspathak Aug 18, 2022

JulieLeeMSFT commented Aug 18, 2022

jeffschwMSFT commented Aug 18, 2022

BruceForstall left a comment

kunalspathak commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

kunalspathak commented Aug 18, 2022

kunalspathak commented Aug 19, 2022

	static bool varTypeNeedsPartialCalleeSave(var_types type)
	{
	assert(type != TYP_STRUCT);
	// ARM64 ABI FP Callee save registers only require Callee to save lower 8 Bytes
	// For SIMD types longer than 8 bytes Caller is responsible for saving and restoring Upper bytes.
	return ((type == TYP_SIMD16) \|\| (type == TYP_SIMD12));
	}

Disable matching constants for vectors that needs upper half to be save/restore #74110

Disable matching constants for vectors that needs upper half to be save/restore #74110

Conversation

kunalspathak commented Aug 17, 2022 • edited Loading

ghost commented Aug 17, 2022

kunalspathak commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulieLeeMSFT commented Aug 18, 2022

jeffschwMSFT commented Aug 18, 2022

BruceForstall left a comment

Choose a reason for hiding this comment

kunalspathak commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

kunalspathak commented Aug 18, 2022

kunalspathak commented Aug 19, 2022

kunalspathak commented Aug 17, 2022 •

edited

Loading

BruceForstall Aug 18, 2022 •

edited

Loading