JIT: add OSR patchpoint strategy, inhibit tail duplication

[AndyAyersMS]

Two changes for OSR:

• add new strategies for placing patchpoints -- either at
  backedge sources (instead of targets) or adaptive. depending
  on number of backedges. Change default to adaptive, since this
  works better with the flow we see from C# for loops.
• inhibit tail duplication for OSR as it may end up interfering
  with loop recognition.

We may not be able to place patchpoints at sources, for various reasons;
if so we fall back to placing them at targets.

[ghost]

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

Issue Details

---

Two changes for OSR:

• add new strategies for placing patchpoints -- either at
  backedge sources (instead of targets) or adaptive depending
  on number of backedges. Change default to sources, since this
  works better with the flow we see from C# for loops.
• inhibit tail duplication for OSR as it may end up interfering
  with loop recognition.

Adaptive placement may end up working out better overall, but
needs further evaluation.

Author:	AndyAyersMS
Assignees:	AndyAyersMS
Labels:	area-CodeGen-coreclr
Milestone:	-

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

[am11]

Would it be clearer to have enum PatchpointStrategy { BackedgeSource, BackedgeTarget, Adaptive }; and return enum value from TC_PatchpointStrategy()?

[AndyAyersMS]

The config system is kind of primitive, so probably not.

[AndyAyersMS]

Looks like this will need some tweaking. We may not be at stack empty point at the top of a source block and so might miss out setting some needed patchpoints.

IIRC ecma requires that IL be stack empty at backedge targets, so the issue hasn't come up before.

Also we may see backedge sources in handler regions; can't put patchpoints there.

[AndyAyersMS]

Ok, should work a bit better now.

[BruceForstall]

It's unfortunate that we have/need bits for this (and BBF_BACKWARD_JUMP_TARGET). Lots of code just compares bbNum, but that depends on bbNum being up-to-date; presumably you can't depend on that? Oh, I guess in addition, you don't want to depend on the preds list being up-to-date and walking the preds list? These bits can obviously themselves get out of date if the flow graph is reordered. Also, do we properly update these bits when, say, we merge blocks and delete the one with the bit?

[AndyAyersMS]

The def/use happens very early, in the flow graph build and importer, before anything else can mess with the flow graph. I could look at source/target nums instead, but that would require enumeration of succs (for sources) and preds (for targets) for every block which would be more costly, and I'd have to build cheap preds.

Nothing later depends on these flag bits. I could scrub them out (at least in debug) if you think leaving them around and potentially gone stale is an attractive nuisance.

Or we could figure out how to have limited-lifetime flags that automatically expire at some point.

[BruceForstall]

It would be useful if we knew when any of the block bits were expected to be valid, either as documentation here, or better, with some kind of checking. Scrubbing them might not be possible (they're only 0/1 already, so scrubbing to 0 doesn't help), unless we add a "valid/invalid" bit for each one, with a debug accessor/checker. Certainly not required with this change (unless you wanted to add comments describing their lifetime).

[BruceForstall]

as => a ?

[AndyAyersMS]

Most failures look like some kind of CI outage or configuration change

##[warning]Docker pull failed with exit code 1, back off 9.857 seconds before retry.
Unable to find image 'mcr.microsoft.com/dotnet-buildtools/prereqs:rhel-7-rpmpkg-c982313-20174116044113' locally

Test failure almost certainly unrelated as OSR is not enabled innerloop except in a handful of runtime tests.

[AndyAyersMS]

Sigh, using github to edit code has its downsides. Will fix.

[AndyAyersMS]

/azp run runtime-jit-experimental

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

OSR stress testing revealed a few issues:

• despite me claiming the flow graph isn't altered and so the flags can be trusted, eh canons can introduce blocks.
• we're setting patchpoints at non-stack-empty block entries, this leads to invalid program errors when the OSR method is jitted because of stack underflows. We need to avoid such patchpoints, which in some IL cases means we will have loops we can't break out of.

Fixes forthcoming.

[AndyAyersMS]

/azp run runtime-jit-experimental

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

Several of bindhandle tests failing in jit-experimental. Can't repro locally so far

      BindHandle call succeeded
      Got wrong error: System.ApplicationException: Error in the application.
         at System.Threading.PortableThreadPool.RegisterForIOCompletionNotifications(IntPtr handle)
         at System.Threading.ThreadPool.BindHandle(SafeHandle osHandle)
         at BindHandle1.RunTest()

There is a concurrent run in main going on right now, will see if it hits those as well.

[jakobbotsch]

The bindhandle failures seem to be somewhat widespread, we hit them in #65682 as well (#65682 (comment)).

[AndyAyersMS]

Yeah -- the jit-experimental run on main also seeing those failures.

[AndyAyersMS]

Looks like both this change and main see 146 failures in jit-experimental.

Spot checked a bunch and all matched, so assuming they exactly overlap.

[AndyAyersMS]

OSX runs seem to be consistently timing out. Nothing here is arch/os specific, and it's almost all off by default. Will ignore.