JIT: Set GTF_ORDER_SIDEEFF for some nodes with invisible dependencies by jakobbotsch · Pull Request #78698 · dotnet/runtime

jakobbotsch · 2022-11-22T14:26:04Z

We have a few places where we create the pattern "COMMA(some check, some value)". In some of these cases there may not be any visible dependency (e.g. use of a defined value) which makes the dependency invisible to the JIT.

If the value is safe to compute only because of the check (for example, a bounds check + indexing operation), and if the value otherwise has no side effects, then nothing prevented the backend or optimizations from reordering these nodes.

A particular problem we may have is around array indexing and bounds checks. Creating an arbitrary illegal byref is not allowed, but to the JIT this node is just a normal node that is completely free of any side effects. Before this change nothing was preventing us from reordering the bounds checks with the computation of the array element.

Fix#78554

We have a few places where we create the pattern "COMMA(some check, some value)". In some of these cases there may not be any visible dependency (e.g. use of a defined value) which makes the dependency invisible to the JIT. If the value is safe to compute only because of the check (for example, a bounds check + indexing operation), and if the value otherwise has no side effects, then nothing prevented the backend or optimizations from reordering these nodes. Fixdotnet#78554

ghost · 2022-11-22T14:26:19Z

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

Issue Details

We have a few places where we create the pattern "COMMA(some check, some value)". In some of these cases there may not be any visible dependency (e.g. use of a defined value) which makes the dependency invisible to the JIT.

If the value is safe to compute only because of the check (for example, a bounds check + indexing operation), and if the value otherwise has no side effects, then nothing prevented the backend or optimizations from reordering these nodes.

Fix #78554

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

jakobbotsch · 2022-11-22T14:41:02Z

 GenTree* lclVar = gtNewLclvNode(lclNum, objRefType);
 GenTree* nullchk = gtNewNullCheck(lclVar, compCurBB);

+ nullchk->gtFlags |= GTF_ORDER_SIDEEFF;


This probably makes #71435 possible again.

a74nh · 2022-11-22T14:58:22Z

Is it worth adding the test case from #78561 ?

jakobbotsch · 2022-11-22T14:59:34Z

Is it worth adding the test case from #78561 ?

Yep, will do that

jakobbotsch · 2022-11-23T14:23:24Z

Diffs were not as bad as I would've feared. The change to check GTF_ORDER_SIDEEFF in if-conversion was the most impactful and we can special case it for invariant nodes/locals, which avoids most of those regressions.

Other than that regressions fell into these categories:

The introduced GTF_ORDER_SIDEEFF on indirections sometimes now disallows RBO from duplicating nodes. It is questionable that we are doing this in the first place, I have opened JIT: RBO still introduces data races #78713 about it.
Setting GTF_ORDER_SIDEEFF on an indir that was the source of a block store would block containment of the source address. Submitted JIT: Ignore source indir when checking interference for block store source address #78763 for this, will push other changes to this PR after this is merged.
Rarely, the new GTF_ORDER_SIDEEFF is propagated to some node that ends up pure later (e.g. due to CSE). This may block address mode creation and other containment that was previously allowed.

jakobbotsch · 2022-11-23T19:07:50Z

Diffs are pretty minimal now, the larger xarch ones are in some of the massive cse tests.

This should unblock #77728.

cc @dotnet/jit-contrib @a74nh PTAL @EgorBo @SingleAccretion

SingleAccretion · 2022-11-23T19:12:27Z

 }
 if (helperNode != nullptr)
 {
+ helperNode->gtFlags |= GTF_ORDER_SIDEEFF;


Why do we need to set the ordering effect on these?
The calls should not be reorderable with GTF_GLOB_REF-tagged field static access by themselves.

Hmm yes, I can revert this one. I just looked for commas and applied this on the op1 in any place there was no directly visible dependency between the operands.

SingleAccretion · 2022-11-23T19:17:48Z

+ nullcheck->gtFlags |= GTF_ORDER_SIDEEFF;
+ GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress);


It is not obvious what the ordering effect here protects against -- what would a theoretical example of bad transformation look like?

I think I can revert this one too given that boxPayloadAddress is known to be very small when cloneOperand is null.
If it wasn't then early prop would probably be able to do something illegal here if it removed this null check due to an upcoming null check, causing us to form some random byref.

Hmm, I'm not sure after all. Without setting this then if we did the optimization in #71435 we could potentially remove these null checks and turn a NRE into an AV when accessing a field right around the explicit null check threshold. Consider something like:
ASGLCL_VAR byref V00COMMA byref // added hereNULLCHECK// node ALCL_VAR ref V01ADD byref LCL_VAR ref V01CNS_INT long 8ASGLCL_VAR int V02IND int // node BADDLCL_VAR byref V00CNS_INT long 0xFFFCASGLCL_VAR int V03IND int // node CLCL_VAR ref V01
The optimization would reason as follows: the node A null check is unnecessary because node B only has the same side effects as the null check (throwing NRE), and node C subsumes node A. However, node B will actually AV without node A.

I would classify this as the optimization being wrong, since we can only consider NRE to be "true" NRE and not "NRE but maybe AV" with small offsets (we do the checks against the max null check offset in other places to differentiate these two cases, though not everywhere).
More fundamentally, fixing this with an ordering effect seems... arbitrary? What is the ordering dependency?

We've talked with @jakobbotsch on Discord and concluded the ordering here is needed because "almost null" byrefs must never occur on dynamically reachable paths, so swapping the ADD with the null check would effectively introduce UB into the program.
It has also been determined that our handling around GTF_ORDER_SIDEEFF is a bit lacking, and it's meaning a bit unclear. We've settled on the "only fully side-effect-less nodes can be reordered with ordered nodes". This then implies both the ADD and the null check need to be tagged as ordered.

It has also been determined that our handling around GTF_ORDER_SIDEEFF is a bit lacking, and it's meaning a bit unclear. We've settled on the "only fully side-effect-less nodes can be reordered with ordered nodes"
It might be useful to add the meaning as a comment. Maybe next to the definition in gentree.h? The current "sub-expression has a re-ordering side effect" is a little lacking.

I agree we should have some more detailed docs on what the effect flags mean and how they are assumed to be set, but I think it would be better to add a section in https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/jit/ryujit-tutorial.md and refer to that. I will aim to do a write-up of that soon.

ok, I'm happy with that.

jakobbotsch · 2022-11-24T12:27:24Z

Diffs after setting GTF_ORDER_SIDEEFF on both operands of the commas are still pretty minimal, with the larger diffs still being test only.

@EgorBo Can you take a look and approve if it looks ok to you?

jakobbotsch · 2022-11-25T12:18:29Z

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

azure-pipelines · 2022-11-25T12:18:54Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2022-11-28T13:52:29Z

Failures are #78912 #78909

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 22, 2022

ghost assigned jakobbotsch Nov 22, 2022

jakobbotsch added 2 commits November 22, 2022 15:27

Flip
c9d66da

Add another
b969e77

jakobbotsch commented Nov 22, 2022
View reviewed changes

jakobbotschand others added 2 commits November 22, 2022 16:43

Run jit-format
ddb5538

Add test
67ee0cb

jakobbotsch mentioned this pull request Nov 22, 2022
JIT: Avoid introducing data races in RBO #78714
Closed

build-analysisBot mentioned this pull request Nov 22, 2022
FileNameStar_NeedsEncoding_EncodedAndDecodedCorrectly test is failing in CI #78718
Closed

jakobbotsch added 2 commits November 23, 2022 13:54

Make if-conversion check less conservative
c2a96d7

Ignore source indir when containing block store source address
bbcd8d5

jakobbotsch mentioned this pull request Nov 23, 2022
JIT: Ignore source indir when checking interference for block store source address #78763
Merged

Merge branch 'main' of github.com:dotnet/runtime into fix-78554
ecc9e03

jakobbotsch marked this pull request as ready for review November 23, 2022 19:07

jakobbotsch requested review from EgorBo and SingleAccretion November 23, 2022 19:08

jakobbotsch commented Nov 23, 2022
View reviewed changes

Comment threadsrc/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.cs Outdated

SingleAccretion reviewed Nov 23, 2022
View reviewed changes

jakobbotschand others added 2 commits November 23, 2022 23:35

Address feedback; add more GTF_ORDER_SIDEEFF flags on other operands
2919001

Update src/tests/JIT/Regression/JitBlue/Runtime_78554/Runtime_78554.cs
57daa31

a74nh mentioned this pull request Nov 24, 2022
Removed range checks have ordered side effects (#78554) #78561
Closed

EgorBo approved these changes Nov 25, 2022
View reviewed changes

jakobbotsch merged commit f35444a into dotnet:mainNov 28, 2022

jakobbotsch deleted the fix-78554 branch November 28, 2022 13:53

a74nh mentioned this pull request Nov 29, 2022
Add simple Else conditions to If Conversion #77728
Merged

dakersnar mentioned this pull request Dec 1, 2022
Regressions in System.Collections.CreateAddAndClear<Int32> #79102
Closed

ghost locked as resolved and limited conversation to collaborators Dec 28, 2022

		nullcheck->gtFlags \|= GTF_ORDER_SIDEEFF;
		GenTree* result = gtNewOperNode(GT_COMMA, TYP_BYREF, nullcheck, boxPayloadAddress);

Uh oh!

Conversation

jakobbotsch commented Nov 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a74nh commented Nov 22, 2022

Uh oh!

jakobbotsch commented Nov 22, 2022

Uh oh!

jakobbotsch commented Nov 23, 2022

Uh oh!

jakobbotsch commented Nov 23, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretionNov 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Nov 24, 2022

Uh oh!

jakobbotsch commented Nov 25, 2022

Uh oh!

azure-pipelinesBot commented Nov 25, 2022

Uh oh!

jakobbotsch commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakobbotsch commented Nov 22, 2022•
edited
Loading

SingleAccretionNov 23, 2022•
edited
Loading