[WinEH] Update CoreCLR EH state numbering

[JosephTremoulet]

Fix the CLR state numbering to generate correct tables for the new funclet
EH representation, and update the lit test to verify them.

Functionally, there are four changes:

• Arity of cleanuppad parameters needs to use getNumArgOperands() instead
  of getNumOperands() now that cleanuppad has a parent token operand.
• We assign a catchswitch the state of its first catch, not its unwind
  destination.
• Don't queue catchpads' users -- this could visit handlers for inner
  tries before handlers for outer tries, and we'll visit everything we
  need to, in correct order, by queuing pads' predecessors. Change the
  initial worklist conditions from isToplevelPadForMSVC to a suitably
  implemented isToplevelPadForCLR accordingly.
• Adjust getEHPadFromPredecessor to allow CLR state numbering to call it
  and have preds returned regardless of their parentage.

Also change some variable names to improve readability.

[JosephTremoulet]

I've updated this to match the latest version of D15325, which means expanding the FI to include r255674 aka b46bb54. Again, the final commit (now ad51af9) shows the non-FI part.
@AndyAyersMS PTAL.

[JosephTremoulet]

This change and dotnet/llilc#968 will need to go in together; http://dotnet-ci.cloudapp.net/view/dotnet_llilc/job/dotnet_llilc_prtest/1344/ is running a test of the combination.

[JosephTremoulet]

http://dotnet-ci.cloudapp.net/view/dotnet_llilc/job/dotnet_llilc_prtest/1344/ is running a test of the combination.

Those tests are failing, so I've been investigating. Part of 8cec2f2 has WinEHPrepare build up a map from invokes to "eh states" stored in the WinEHFuncInfo, which is later consulted by SelectionDAGBuilder when lowering invokes. In LLILC, RewriteStatepointsForGC swoops in between and, for all invokes which are GC safepoints, replaces the original invoke with an invoke of @llvm.gc.statepoint that takes the callee as an argument, which severs the connection with the map in the WinEHFuncInfo. :/

Seems to be a classic case of two different things trying to "run last", but it's not immediately obvious how to improve on that without teaching one directly about the details of the other... maybe the InvokeStateMap should be a ValueMap so that its keys will be rewritten when the safepoint rewriting calls replaceAllUsesWith.

[JosephTremoulet]

maybe the InvokeStateMap should be a ValueMap so that its keys will be rewritten when the safepoint rewriting calls replaceAllUsesWith.

Not that simple; the lookup goes through a CallLoweringInfo object that also can't find the original invoke...

[AndyAyersMS]

Hmm, side tables strike again.

[JosephTremoulet]

I can reproduce this on e.g. JIT\Methodical\explicit\coverage\expl_float_1_r\expl_float_1_r with conservative GC. @swaroop-sridhar, is it expected that we'd be trying to lower statepoints with conservative GC?

[swaroop-sridhar]

Even with conservative GC, Statepoints are inserted and lowered for GC-Transitions.
But the insertion happens in the reader -- PlaceSafepoints and RewriteStatepointsforGC phases are not run if DoInsertStatepoints == false. These statepoints without any actual gc-pointers tracked within them, and lowered as usual.

[JosephTremoulet]

Ah, ok, thanks. Then the issue is purely that the CallLoweringInfo constructed by lowerStatepoint doesn't link in the CallSite, which lowerCallOperands now demands on invokes in functions with funclets. Need to understand why lowerStatepoint is using that overload to construct the CallLoweringInfo and whether there's a better channel to pass that information...

[JosephTremoulet]

I did some poking around. The two means of creating CallLoweringInfos (i.e. with and without the CallSite) have existed for as long as CallLoweringInfo has. The EH state map needs to use InvokeInsts as keys now because there are no longer EndPads and so it can't just use the unwind destination like it did previously. The InvokeInst is needed in SelectionDAGBuilder::lowerInvokable, which is a private helper method. It has two callers, one of which is for statepoints and the other of which always passes a CallLoweringInfo with a CallSite. To me, it makes more sense to just add the InvokeInst as a parameter to the helper, to avoid upsetting expectations in the call lowering code about what it means when a CallLoweringInfo has a CallSite attached to it and to avoid expanding CallLoweringInfo to include EH state map info that's only relevant for funclet EH. So, I've pushed a change does just that and would like to merge it. I'd have submitted the same for upstream review, except that it ought to be accompanied by a lit test, and reproducing the error requires a statepoint invoke which unwinds to a funclet EH pad, and the upstream statepoint lowering code fails elsewhere first on that combination -- #106 stubbed out those paths so they don't crash, but I think it makes sense to upstream this change only after the stubs have been replaced with a real implementation (tracked by #105) and that has been upstreamed as well.

@AndyAyersMS PTAL. The only change is the additional commit.

[AndyAyersMS]

Latest commit LGTM.