Automate Diffing of Release, Checked and Debug JITs #5884
Assignees
Labels
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
test-enhancement
Improvements of test source code
Milestone
Comments
|
dotnet/coreclr#5286 is evidence that just doing mscorlib may not be sufficient. That difference between release/checked is due to invalid IL, so perhaps not the most compelling examples, but we should certainly consider larger scope if the cost isn't prohibitive. |
AndyAyersMS
referenced
this issue
in AndyAyersMS/coreclr
Jul 21, 2016
Closes #6368. optMethodFlags is a member field of the Compiler object. It is used during importation to flag IR that might benefit from earlyProp, and used in earlyProp to skip execution if nothing is flagged. However, there were a number of issues with the implementation. First, the value was never initialized. So in CHK/DBG builds, it would get set to 0xFFFFFFFF by the default allocator fill. In RELEASE builds the value would be randomly set. This randomness could easily lead to nondeterministic codegen in RELEASE. More on this subsequently. Second, the value was not propagated during inlining. So if interesting constructs only entered the root method via inlines, they potentially might not trigger earlyProp. Third, not all interesting constructs were flagged. The JIT ORs flag values into optMethodFlags as it imports constructs that should trigger earlyProp. It also re-uses the same compiler instance in most cases. So, relatively quickly, even in release, all the relevant flags end up set and earlyProp will always be run. Hence the window for observing the nondeterministic is was limited to the first few methods jitted by each compiler instance. So to sum up: in DBG/CHK, early prop always runs, regardless of need. In RELEASE, it runs unpredictably for the first few methods, then always runs, regardless of need. To see the nondeterminism in RELEASE, early methods would have to be free of interesting constructs but pull them in via inlining (or contain interesting constructs not currently flagged as such during importation). This set of circumstances is evidently rare enough that the nondeterminism has not been noted. Also, it is quite unlikely to lead to bad codegen, just missed opportunities. We might have caught this if we had reliable RELEASE/CHK diffing available, eg as in #5063. The fix is to initialize optMethodFlags to zero in compInit, and merge in the inlinee's copy during fgInsertInlineeBlocks. Logging tracks when the inlinee's copy causes a flag update. This was tricky to test for diffs because in CHK all that should happen is that we run earlyProp less often. So any diff in CHK is a missed case of flagging during importation. I found once such case via SPMI and added constant indices to the flagging set. With this change in place, earlyProp how runs selectively based on need in all configurations. No diffs from CHK so the cases where we don't run earlyProp now are cases where it was just wasting time before.
AndyAyersMS
referenced
this issue
in AndyAyersMS/coreclr
Jul 21, 2016
Closes #6368.
optMethodFlags is a member field of the Compiler object. It is used
during importation to flag IR that might benefit from earlyProp, and
used in earlyProp to skip execution if nothing is flagged. However,
there were a number of issues with the implementation.
First, the value was never initialized. So in CHK/DBG builds, it would
get set to 0xFFFFFFFF by the default allocator fill. In RELEASE builds
the value would be randomly set. This randomness could easily lead to
nondeterministic codegen in RELEASE. More on this subsequently.
Second, the value was not propagated during inlining. So if interesting
constructs only entered the root method via inlines, they potentially
might not trigger earlyProp.
Third, not all interesting constructs were flagged.
The JIT ORs flag values into optMethodFlags as it imports constructs
that should trigger earlyProp. It also re-uses the same compiler instance
in most cases. So, relatively quickly, even in release, all the relevant
flags end up set and earlyProp will always be run. Hence the window
for observing the nondeterministic is was limited to the first few
methods jitted by each compiler instance.
So to sum up: in DBG/CHK, early prop always runs, regardless of need.
In RELEASE, it runs unpredictably for the first few methods, then always
runs, regardless of need.
To see the nondeterminism in RELEASE, early methods would have to be free
of interesting constructs but pull them in via inlining (or contain
interesting constructs not currently flagged as such during importation).
This set of circumstances is evidently rare enough that the nondeterminism
has not been noted. Also, it is quite unlikely to lead to bad codegen,
just missed opportunities.
We might have caught this if we had reliable RELEASE/CHK diffing ready;
see for instance #5063.
The fix is to initialize optMethodFlags to zero in compInit, and merge in
the inlinee's copy during fgInsertInlineeBlocks. Logging tracks when the
inlinee's copy causes a flag update.
This was tricky to test for diffs because in CHK all that should happen is
that we run earlyProp less often. So any diff in CHK is a missed case of
flagging during importation. I found once such case via SPMI where the array
index is a constant, and added constant indices to the flagging set.
Because I also set the block flag, this both caused earlyProp to run
and also look at that block; the latter has triggered a fair number of
diffs, almost all of them beneficial (0.42% improvement in SPMI), winners
outnumber losers by about 50:1.
A typical example of a diff is that the null check below is removed.
```asm
mov rcx, qword ptr [(reloc)]
mov edx, 4
call CORINFO_HELP_NEWARR_1_VC
** mov edx, dword ptr [rax+8]
mov byte ptr [rax+16], 255
mov byte ptr [rax+17], 254
```
With this change in place, earlyProp how runs selectively based on need in
all configurations. We're still running earlyProp in all the cases where it
can make a difference, and finding more improvements when it does run, but
we're not running it in cases where it can't.
|
Ideally we would use |
RussKeldorph
changed the title
|
@RussKeldorph seems unlikely we'll get to this in 2.1. Or do you have some secret plan I don't know about? |
|
No secret plan. Just wishful thinking maybe... Agreed this will probably move out in the next round of triage. |
|
Added Checked and Release diff in AzDo pipeline by #61335. |
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.
Create a script to verify there's no difference in jit outputs for check versus debug builds and checked vs release builds. We have verified this locally, but we need to automate it.
category:correctness
theme:testing
skill-level:intermediate
cost:medium
The text was updated successfully, but these errors were encountered: