This is an archive of the discontinued LLVM Phabricator instance.

[WinEH] Calculate state numbers for the new EH representation
ClosedPublic

Authored by majnemer on Aug 18 2015, 2:02 AM.

Details

Summary

State numbers are calculated by performing a walk from the innermost
funclet to the outermost funclet. Rudimentary support for the new EH
constructs has been added to the assembly printer, just enough to test
the new machinery.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 32388.Aug 18 2015, 2:02 AM
majnemer retitled this revision from to [WinEH] Calculate state numbers for the new EH representation.
majnemer updated this object.
majnemer added reviewers: JosephTremoulet, rnk.
majnemer added a subscriber: llvm-commits.

LGTM, though I'm less familiar with this part of the code. A few nits/questions inline, but all just minor take-it-or-leave-it suggestions.

lib/CodeGen/AsmPrinter/WinException.cpp
185–188

Nit: I don't understand this change; the function will fail if you pass it a non-null non-GlobalValue Value, so to me it seems less error prone to type the parameter as GlobalValue and force callers to recognize the need to downcast.

lib/CodeGen/WinEHPrepare.cpp
2581

Nit: const_cast is unfortunate. Would it be horrible to push constness down or non-constness up? (I'm not familiar with the callers/callees imposing the restrictions here)

2949–2958

Nit: this was a confusing read because it's not getting the unwind pred of BB, it's getting the unwind pred of a successor of BB. I think it would be easier to read if the parameter name were PredBB and/or a comment explained.

3344

I don't understand this change, is there some style guideline against periods ending comments? The other comments in the file tend to have them, and this one is even a full sentence.

rnk edited edge metadata.Aug 18 2015, 9:12 AM

Much excitement. :)

lib/CodeGen/AsmPrinter/WinException.cpp
185–188

Right, the intent is for the assertion to fail at this point once we start passing such values through. Previously, we outlined funclets in IR, and the IR function was the GlobalValue that we passed through. With the new representation, we are passing a BasicBlock, which inherits from Value. We can't actually reach this code today because SDAGBuilder can't select our new pad instructions yet. Eventually we will get here, the assertion will fail, and we'll fix it up and give it the correct BasicBlock type.

lib/CodeGen/WinEHPrepare.cpp
2581

Pushing it down is the right thing in this case. The unwind map lives through codegen, and there shouldn't be non-const IR pointers hanging around during codegen.

2949–2958

I guess it really does the following:

Given BB which ends in an unwind edge, return the EHPad that this BB belongs to. If the unwind edge came from an invoke, return null.

Call it getEHPadFromPredecessor?

3284

Also left over from hurried pair programming. :)

lib/Target/X86/X86WinEHState.cpp
400–405

Probably worth commenting that we only ever create our own WinEHFuncInfo for testing purposes. MMI is non-null iff we are running as part of a CodeGen pass manager.

Given that it's for testing only... we should heap allocate it with something like this:

// If MMI is null, create our own WinEHFuncInfo. This only happens in opt tests.
std::unique_ptr<WinEHFuncInfo> FuncInfoPtr;
if (!MMI)
  FuncInfoPtr.reset(new WinEHFuncInfo());
WinEHFuncInfo &FuncInfo = *(MMI ? &MMI->getWinEHFuncInfo(&F) : FuncInfoPtr.get());
496–501

Both of these functions only consume WinEHFuncInfo. We should hoist this logic up to runOnFunction and only pass down WinEHFuncInfo.

majnemer updated this revision to Diff 32430.Aug 18 2015, 11:10 AM
majnemer edited edge metadata.

Address review comments.

rnk accepted this revision.Aug 18 2015, 11:48 AM
rnk edited edge metadata.

lgtm

include/llvm/CodeGen/WinEHFuncInfo.h
125

This guy should be const. I'm reasonably confident that the const cascading will stop there, since all we use it for is to get an MCSymbol.

lib/CodeGen/WinEHPrepare.cpp
2610

We should kill this const_cast too.

This revision is now accepted and ready to land.Aug 18 2015, 11:48 AM
This revision was automatically updated to reflect the committed changes.
ivladak added a subscriber: ivladak.Sep 1 2015, 6:35 AM

Could you please explain why you

initializeWinEHStatePassPass(PR);

in LLVMInitializeX86Target() regardless of whether the target is win32 or not?

Sorry if the question is silly. I actually have NFC about what's going on in this commit.

I am getting some nasty memory corruption problems which might or might not be caused by this unconditional initialization.

rnk added a subscriber: rnk.Sep 1 2015, 9:05 AM

We have no idea at initialization time if the user wants to target win32 or
the host, so we couldn't conditionalize it if we wanted to.

We initialize the pass so that we can run it from opt and test it in
isolation. It's consistent what NVPTX does in
llvm/lib/Target/NVPTX/NVPTXTargetMachine.cpp. We just copied the technique.