Jit: fix issues with optMethodFlags
authorAndy Ayers <andya@microsoft.com>
Thu, 21 Jul 2016 07:34:25 +0000 (00:34 -0700)
committerAndy Ayers <andya@microsoft.com>
Thu, 21 Jul 2016 23:44:22 +0000 (16:44 -0700)
commitf1b03b6a6f993331acc1da40b0e630348a3abaea
tree40f00d3b93207e0f0d8f2fc7e0b214ff886d16d7
parent32a020e47a0797bba4167e85efc38a34397f633e
Jit: fix issues with optMethodFlags

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.
src/jit/compiler.cpp
src/jit/compiler.h
src/jit/flowgraph.cpp
src/jit/importer.cpp