JIT: Relax reg-optionality validity checks #81614

jakobbotsch · 2023-02-03T20:55:03Z

This PR does two things:

Rename the existing IsSafeToContainMem into IsInvariantInRange, and use IsInvariantInRange in the places where we are using IsSafeToContainMem today as a general-purpose "can move" check. IsSafeToContainMem remains and just forwards to IsInvariantInRange. We still use it for the cases where we're actually containing a load. This is follow-up to Add lowering support for conditional nodes #71705 (comment) that I promised a long time ago.
Relax reg-optionality checking. The checks we have today are much more stringent than they need to be, doing a full (and expensive) invariance check. Let me copy/paste my comment from the new IsSafeToMarkRegOptional about the checking we actually need to do:

//    Unlike containment, reg-optionality can only rarely introduce new
//    conflicts, because reg-optionality mostly does not cause the child node
//    to be evaluated at a new point in time:
//
//    1. For LIR edges (i.e. anything that isn't GT_LCL_VAR) reg-optionality
//       indicates that if the edge was spilled to a temp at its def, the parent
//       node can use it directly from its spill location without reloading it
//       into a register first. This is always safe as as spill temps cannot
//       interfere.
//
//       For example, an indirection can be marked reg-optional even if there
//       is interference between it and its parent; the indirection will still
//       be evaluated at its original position, but if the value is spilled to
//       stack, then reg-optionality can allow using the value from the spill
//       location directly.
//
//    2. For GT_LCL_VAR reg-optionality indicates that the node can use the
//       local directly from its home location. IR invariants guarantee that the
//       local is not defined between its LIR location and the parent node (see
//       CheckLclVarSemanticsHelper). That means the only case where it could
//       interfere is due to it being address exposed. So this is the only unsafe
//       case.
//

ghost · 2023-02-03T20:55:38Z

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

Issue Details

null

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

src/coreclr/jit/lower.cpp

jakobbotsch · 2023-02-04T02:01:50Z

/azp run Fuzzlyn

azure-pipelines · 2023-02-04T02:02:00Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2023-02-04T16:32:58Z

/azp run Fuzzlyn

azure-pipelines · 2023-02-04T16:33:12Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2023-02-06T10:52:39Z

This will conflict with #81267 so will wait for that one to get in first. It should still be ready for review though, cc @dotnet/jit-contrib PTAL @BruceForstall @kunalspathak

SingleAccretion · 2023-02-06T10:57:45Z

src/coreclr/jit/lower.cpp

+//
+bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) const
+{
+    if (!childNode->OperIs(GT_LCL_VAR))


It is a rather subtle difference between LCL_VAR and LCL_FLD that LCL_FLDs, if marked reg optional (which they won't be under normal circumstances), will use spill temps. Something that would be good to write down explicitly I think.

I can add it with the "indirection" example above? I suppose it's the exact same example.

Yes, just mentioning it would be good.

BruceForstall

LGTM

BruceForstall · 2023-02-08T23:47:19Z

src/coreclr/jit/lower.cpp

-//    true if it is safe to make childNode a contained memory operand.
+// Returns:
+//    True if 'node' can be evaluated at any point between its current
+//    location and 'parentNode' without giving a different result; otherwise


Suggested change

// location and 'parentNode' without giving a different result; otherwise

// location and 'endExclusive' without giving a different result; otherwise

…nality

jakobbotsch · 2023-02-09T13:41:59Z

The Fuzzlyn failure was #75442.

jakobbotsch · 2023-02-09T13:42:47Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn, runtime-coreclr jitstressregs, runtime-coreclr libraries-jitstressregs

azure-pipelines · 2023-02-09T13:43:14Z

Azure Pipelines successfully started running 5 pipeline(s).

tannergooding · 2023-02-09T14:01:43Z

src/coreclr/jit/lower.cpp

 {
+    assert((node != nullptr) && (endExclusive != nullptr));
+


Is there any invariant case we should handle here? That is, the prior handles the linear order being node, endExclusive. Should this handle node, ignoreNode, endExclusive?

I'll add an additional assert about this

tannergooding · 2023-02-09T14:09:31Z

src/coreclr/jit/lower.cpp

+//       interfere is due to it being address exposed. So this is the only unsafe
+//       case.
+//
+bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) const


Is any of this optimization that should also apply to IsSafeToContainMem?

That is, iirc there is a "pessimization" for the InterferesWith check today with regards to locals in that any write is considered interfering, even if its to another local and therefore cannot impact the local being read.

However, based on https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md I believe we are allowed to reorder such a non-volatile read of a local assuming there are no volatile writes.

Maybe what I'm remembering is just the fact that we couldn't mark it reg-optional and the fix is exactly what is being handled here, not something to the general IsSafeToContainMem

I don't think most of this applies to IsSafeToContainMem, the problem there is really that it causes evaluation to happen at a new point in time, which brings along with it all of the extra checking. W.r.t the locals, from looking at AliasSet::InterferesWith it looks like we are pretty precise:

runtime/src/coreclr/jit/sideeffects.cpp

Lines 319 to 335 in 917f407

//------------------------------------------------------------------------

// AliasSet::InterferesWith:

// Returns true if the reads and writes in this alias set interfere

// with the given alias set.

//

// Two alias sets interfere under any of the following conditions:

// - Both sets write to any addressable location (e.g. the heap,

// address-exposed locals)

// - One set reads any addressable location and the other set writes

// any addressable location

// - Both sets write to the same lclVar

// - One set writes to a lclVar that is read by the other set

//

// Arguments:

// other - The other alias set.

//

bool AliasSet::InterferesWith(const AliasSet& other) const

@SingleAccretion managed to find the discussion we had it some time back on Discord

The general scenario I was thinking about is: #76273 (comment)

Many diffs are cases like the following:

- add x0, x0, w26, UXTW - ldrb w0, [x0] + ldrb w0, [x0, w26, UXTW #2]

These seem to mostly be due to ADDEX not being properly handled in many places while ADD always is.

There are a few small regressions such as:

+ lsl w0, w0, #8 ldr x1, [fp, #0xA8] // [V192 tmp170] ldrb w1, [x1, #0x01] - add w1, w1, w0, LSL #8 + add w1, w0, w1

Notably we're checking if childNode->gtGetOp1() (the value to be shifted) is contained, the original doesn't (but it shouldn't be anyways since we need a register for it).

We're also checking IsSafeToContainMem(parentNode, childNode) where-as the original wasn't. I expect this is the "actual" reason for the "larger diff" since we always do "strict" checking and we don't allow reordering across anything with a "side effect". That being said, there is no actual interdependence between these two and so it is technically "safe" anyways.

jakobbotsch · 2023-02-09T23:12:19Z

The jitstress/jitstressregs failures are all present in the previous rolling run too.

I think there's something wrong with arm64-OSX exception handling in general on current main, I'm seeing a lot of Fuzzlyn failures too. According to those the runtime is crashing after printing "Unhandled exception", just like those failures in jitstress/jitstressregs. (Note that Fuzzlyn catches all exceptions, so this is unexpected.)
@janvorli is this a known issue?

Edit: the symptom is the same as in #81869

tannergooding · 2023-02-09T23:20:26Z

outerloop commit range between last success and first failure: f01d5a0...215839e

janvorli · 2023-02-09T23:51:02Z

Sigh, I guess I know what's wrong. My change to prevent unwinding through the bottom of the stack on Alpine is incorrect for secondary threads.

jakobbotsch · 2023-02-10T20:52:48Z

/azp run Fuzzlyn

azure-pipelines · 2023-02-10T20:53:04Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2023-02-13T12:42:58Z

I can reproduce the problematic codegen in the macOS fuzzlyn example on main too. Will open an issue and investigate that separately.

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 3, 2023

ghost assigned jakobbotsch Feb 3, 2023

jakobbotsch added 2 commits February 4, 2023 01:35

JIT: Relax reg-optionality validity checks

cf0952b

Fix bad refactoring

8df13a7

jakobbotsch force-pushed the relax-reg-optionality branch from e97e020 to 8df13a7 Compare February 4, 2023 01:04

tannergooding reviewed Feb 4, 2023

View reviewed changes

src/coreclr/jit/lower.cpp Show resolved Hide resolved

Add a couple of asserts in IsInvariantInRange

bee4fb7

Add function header

dc7f280

SingleAccretion reviewed Feb 6, 2023

View reviewed changes

BruceForstall approved these changes Feb 8, 2023

View reviewed changes

jakobbotsch added 2 commits February 9, 2023 14:40

Address feedback

7ac5c9c

Merge branch 'main' of github.com:dotnet/runtime into relax-reg-optio…

fcc9a9f

…nality

jakobbotsch marked this pull request as ready for review February 9, 2023 13:42

tannergooding reviewed Feb 9, 2023

View reviewed changes

tannergooding approved these changes Feb 9, 2023

View reviewed changes

jakobbotsch mentioned this pull request Feb 9, 2023

Try some relaxed interference checks #81892

Closed

jakobbotsch merged commit faab048 into dotnet:main Feb 13, 2023

jakobbotsch deleted the relax-reg-optionality branch February 13, 2023 12:45

ghost locked as resolved and limited conversation to collaborators Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Relax reg-optionality validity checks #81614

JIT: Relax reg-optionality validity checks #81614

jakobbotsch commented Feb 3, 2023 •

edited

Loading

ghost commented Feb 3, 2023

jakobbotsch commented Feb 4, 2023

azure-pipelines bot commented Feb 4, 2023

jakobbotsch commented Feb 4, 2023

azure-pipelines bot commented Feb 4, 2023

jakobbotsch commented Feb 6, 2023

SingleAccretion Feb 6, 2023

jakobbotsch Feb 6, 2023

SingleAccretion Feb 6, 2023

BruceForstall left a comment

BruceForstall Feb 8, 2023

jakobbotsch commented Feb 9, 2023

jakobbotsch commented Feb 9, 2023

azure-pipelines bot commented Feb 9, 2023

tannergooding Feb 9, 2023

jakobbotsch Feb 9, 2023

tannergooding Feb 9, 2023

tannergooding Feb 9, 2023 •

edited

Loading

jakobbotsch Feb 9, 2023 •

edited

Loading

tannergooding Feb 9, 2023

jakobbotsch commented Feb 9, 2023 •

edited

Loading

tannergooding commented Feb 9, 2023

janvorli commented Feb 9, 2023

jakobbotsch commented Feb 10, 2023

azure-pipelines bot commented Feb 10, 2023

jakobbotsch commented Feb 13, 2023

	// location and 'parentNode' without giving a different result; otherwise
	// location and 'endExclusive' without giving a different result; otherwise

	//------------------------------------------------------------------------
	// AliasSet::InterferesWith:
	// Returns true if the reads and writes in this alias set interfere
	// with the given alias set.
	//
	// Two alias sets interfere under any of the following conditions:
	// - Both sets write to any addressable location (e.g. the heap,
	// address-exposed locals)
	// - One set reads any addressable location and the other set writes
	// any addressable location
	// - Both sets write to the same lclVar
	// - One set writes to a lclVar that is read by the other set
	//
	// Arguments:
	// other - The other alias set.
	//
	bool AliasSet::InterferesWith(const AliasSet& other) const

JIT: Relax reg-optionality validity checks #81614

JIT: Relax reg-optionality validity checks #81614

Conversation

jakobbotsch commented Feb 3, 2023 • edited Loading

ghost commented Feb 3, 2023

jakobbotsch commented Feb 4, 2023

azure-pipelines bot commented Feb 4, 2023

jakobbotsch commented Feb 4, 2023

azure-pipelines bot commented Feb 4, 2023

jakobbotsch commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Feb 9, 2023

jakobbotsch commented Feb 9, 2023

azure-pipelines bot commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

jakobbotsch Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Feb 9, 2023 • edited Loading

tannergooding commented Feb 9, 2023

janvorli commented Feb 9, 2023

jakobbotsch commented Feb 10, 2023

azure-pipelines bot commented Feb 10, 2023

jakobbotsch commented Feb 13, 2023

jakobbotsch commented Feb 3, 2023 •

edited

Loading

tannergooding Feb 9, 2023 •

edited

Loading

jakobbotsch Feb 9, 2023 •

edited

Loading

jakobbotsch commented Feb 9, 2023 •

edited

Loading