JIT: remove GTF_INX_REFARR_LAYOUT #33098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
maryamariyan
added
the
area-CodeGen-coreclr
|
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. |
|
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. |
briansull
reviewed
Mar 6, 2020
| 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.
It looks like you deleted the place below that uses this bool, is that intentional?
briansull
reviewed
Mar 6, 2020
| if (hasZeroMapAnnotation) | ||
| { | ||
| m_pCompiler->fgAddFieldSeqForZeroOffset(ref, fldSeq); | ||
| } |
There was a problem hiding this comment.
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. |
erozenfeld
approved these changes
Mar 7, 2020
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.
Suggested change
| // If there's a bounds check, the the indir won't fault. | |
| // If there's a bounds check, then the indir won't fault. |
|
Can revisit the |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When morphing
GT_INDEXnodes, we were inadvertently also settingGTF_IND_NONFAULTINGfor theGT_INDsubtree for ref type arrays, becauseGTF_IND_NONFAULTINGhas 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_INDcan't fault is ok.A no diff change would remove the
GTF_INX_REFARR_LAYOUTflag and then modifyfgMorphArrayIndexto setGTF_IND_NONFAULTINGfor 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_EXCEPTwhen 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.