-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: remove GTF_INX_REFARR_LAYOUT #33098
JIT: remove GTF_INX_REFARR_LAYOUT #33098
Conversation
When morphing `GT_INDEX` nodes, we were inadvertently also setting `GTF_IND_NONFAULTING` for the `GT_IND` subtree for ref type arrays, because `GTF_IND_NONFAULTING` has the same value as `GTF_INX_REFARR_LAYOUT`. This turns out to be safe since in general there is an upstream bounds check to cover the null check from the indexing operation, so the fact that we were claiming the `GT_IND` can't fault is ok. A no diff change would remove the `GTF_INX_REFARR_LAYOUT` flag and then modify `fgMorphArrayIndex` to set `GTF_IND_NONFAULTING` for ref type arrays with bounds checks: ``` // If there's a bounds check, the the indir won't fault. if (bndsChk && (tree->gtType == TYP_REF)) { tree->gtFlags |= GTF_IND_NONFAULTING; } tree->gtFlags |= GTF_EXCEPT; ``` But there's no good reason to limit the above change to ref type arrays and no good reason to OR in `GTF_EXCEPT` when there are bounds checks. Once we do the more general fix we see diffs, so we might as well further clean up the related constraint in the importer found under `REDO_RETURN_NODE`. Closes dotnet#32647.
cc @dotnet/jit-contrib This causes a small regression. I sampled a few of the more prominent ones, and in those we do extra CSEs and loop hoists and then often see different allocation patterns. Some of the regressions might be fixable by renumbering blocks before LSRA, because late flow graph updates after liveness cause divergence between the block number order and flow order. I know I've seen this before... will look at it as a separate issue.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test failures look like they're caused by this change; investigating. |
Turns out this simplification is not safe to enable more broadly, but not for the reasons stated in the comment: runtime/src/coreclr/src/jit/importer.cpp Lines 9101 to 9118 in e66e9fa
It can lose information about the type of the array elements and so hit an assert in value numbering.
so will undo the code change and update the comment. |
1 similar comment
This comment has been minimized.
This comment has been minimized.
Now hitting an assert on some x86 library tests:
Will investigate. |
Having some trouble reproing the failure above.... since an earlier commit didn't hit this, am going to try merging up locally to see if that's what is needed. |
Still not able to repro, am going to try rerunning these... |
Got a repro, finally. Looks like the CSE code is running into trouble with GT_INDs that should be CSEs but have different types, here TYP_STRUCT and TYP_REF.
We can legitimately see this lind of IR for cases where structs are returned as various native types (say a struct-wrapped TYP_REF) and we inline. This PR enables more array element CSEs because we mark more array accesses as nonfaulting, so not too surprising we're hitting this here (previously only TYP_REF accesses were so blessed so we weren't seeing so many mixed cases). I think the fix is along the lines of the following: adjust CSE's "guess the handle" rules so that if one tree is TYP_STRUCT and another is TYP_REF we just forgo CSEing them for now. |
Simple repro for the failure (release x86) using System;
using System.Collections.Generic;
struct RefAsValueType<T>
{
public T Value;
public RefAsValueType(T value)
{
this.Value = value;
}
}
class X
{
public static int Main()
{
string a = "hello, world!";
var av = new RefAsValueType<string>(a);
var stack = new Stack<RefAsValueType<string>>();
stack.Push(av);
string b = stack.Pop().Value;
return a == b ? 100 : -1;
}
} |
In theory, we don't have type information on indirection node themselves, it is contained in the child trees. Note that it is different for |
IR Dumps for base jit and diff jit. This seems to be related to your change to not eagerly retype return values, since that's where part of the divergence comes in, one of the CSE trees (TYP_REF) is retyped, the other (TYP_STRUCT) is not. In the baseline jit, we don't mark the TYP_STRUCT indir as nonfaulting -- we just do it (inadvertently) for the TYP_REF indir. So this causes CSE to back away.
but with this PR both INDs are nonfaulting, CSE tries to carry on, and blows up with incompatible types. |
Maybe this is related: morph is putting a field sequence on a COMMA, perhaps it was meant for the underlying ADDR.
|
I do not think that it is related to return-call-retyping. From what I see
So I think the real issue is in
if you ask for I want to take a closer look at this case, but right now my enlistment is screwed with the latest build changes. |
I could be getting confused, I guess it was the first version of this PR that had a return-retyping issue. Changing morph so that the field seq ends up on the ADDR instead of the COMMA fixes this latest issue. |
Now we're attempting some redundant annotations.
|
That looks similar to https://github.com/dotnet/runtime/pull/32085/files#diff-8ed3af14b61694b641239e8e88b1a762. runtime/src/coreclr/src/jit/valuenum.cpp Lines 7527 to 7531 in 82508ca
? |
Haven't debugged yet; will let you know soon. |
No, not the same issue. CSE is checking if it needs to add a zero offset field seq, and so now also needs to tunnel through commas to get at the actual node of interest.
|
bool hasZeroMapAnnotation = m_pCompiler->GetZeroOffsetFieldMap()->Lookup(exp, &fldSeq); | ||
bool commaOnly = true; | ||
GenTree* effectiveExp = exp->gtEffectiveVal(commaOnly); | ||
const bool hasZeroMapAnnotation = m_pCompiler->GetZeroOffsetFieldMap()->Lookup(effectiveExp, &fldSeq); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you deleted the place below that uses this bool, is that intentional?
if (hasZeroMapAnnotation) | ||
{ | ||
m_pCompiler->fgAddFieldSeqForZeroOffset(ref, fldSeq); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, originally there were two places the zero map annotation got added -- in the def case, and below the join of the use/def. So it was possible to try and add the zero map annotation to the def twice.
Now we just rely on the common one below the join.
x86 test failure seems to be an instance of #33276. |
Updated diffs, similar to before..
@dotnet/jit-contrib anyone else have feedback? Think this is ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just spotted one comment typo.
src/coreclr/src/jit/morph.cpp
Outdated
@@ -5552,8 +5552,15 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) | |||
// This is an array index expression. | |||
tree->gtFlags |= GTF_IND_ARR_INDEX; | |||
|
|||
/* An indirection will cause a GPF if the address is null */ | |||
tree->gtFlags |= GTF_EXCEPT; | |||
// If there's a bounds check, the the indir won't fault. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If there's a bounds check, the the indir won't fault. | |
// If there's a bounds check, then the indir won't fault. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Can revisit the |
When morphing
GT_INDEX
nodes, we were inadvertently also settingGTF_IND_NONFAULTING
for theGT_IND
subtree for ref type arrays, becauseGTF_IND_NONFAULTING
has the same value asGTF_INX_REFARR_LAYOUT
.This turns out to be safe since in general there is an upstream bounds check to
cover the null check from the indexing operation, so the fact that we were
claiming the
GT_IND
can't fault is ok.A no diff change would remove the
GTF_INX_REFARR_LAYOUT
flag and then modifyfgMorphArrayIndex
to setGTF_IND_NONFAULTING
for ref type arrays with boundschecks:
But there's no good reason to limit the above change to ref type arrays and no
good reason to OR in
GTF_EXCEPT
when there are bounds checks.Once we do the more general fix we see diffs, so we might as well further clean
up the related constraint in the importer found under
REDO_RETURN_NODE
.Closes #32647.