This is an archive of the discontinued LLVM Phabricator instance.

Remap frame variables used in outlined C++ exception handlers
ClosedPublic

Authored by andrew.w.kaylor on Feb 19 2015, 2:02 PM.

Details

Summary

These changes implement the code to find values referenced inside outlined catch and cleanup handlers and relocate them to a frame allocation structure that is shared by the parent function and the handlers.

During outlining, temporary allocas are created in the handler function to represent the variable. If the value referenced was an alloca in the parent frame, the mapping is simple. Otherwise, the value will need to be spilled. (Spilling isn't working yet.) As values are referenced in handlers, they are added to a map that will be used for resolution later.

After all handlers have been outlined, we walk the map and add each referenced variable to a structure definition and store the index for the corresponding element. When this is complete, we add bitcasts to all of the handlers (which already have an instruction to get a raw pointer to the frame allocation block), create a frame allocation call in the parent function and finally replace both original and temporary allocas with GEPs.

In the case of an alloca which is referenced from only one handler and is not used in the parent frame after the outlined function have been pruned, the alloca is simply erased from the parent and left in the handler. No entry in the frame allocation structure is created for such variables. (This will probably require some additional handling once spills are working.)

Diff Detail

Repository
rL LLVM

Event Timeline

andrew.w.kaylor retitled this revision from to Remap frame variables used in outlined C++ exception handlers.
andrew.w.kaylor updated this object.
andrew.w.kaylor edited the test plan for this revision. (Show Details)
andrew.w.kaylor added reviewers: rnk, majnemer, ABataev.
andrew.w.kaylor set the repository for this revision to rL LLVM.
andrew.w.kaylor added a subscriber: Unknown Object (MLST).
rnk added inline comments.Feb 19 2015, 3:49 PM
lib/CodeGen/WinEHPrepare.cpp
43 ↗(On Diff #20337)

SmallVectors don't nest in other containers very efficiently. You can use a TinyPtrVector instead, which just one pointer, and can be moved very cheaply during a container reallocation.

I'm also partial to just making a struct for the map values instead of using std::pair, along the lines of:

struct HandlerAllocas {
  TinyPtrVector<AllocaInst> Allocas;
  int ParentFrameAllocationIndex;
};
245 ↗(On Diff #20337)

Can we sort FrameVarInfo by the alignment and size of ParentAlloca first? It doesn't have to be this patch, but a FIXME here would be good. Since we're freezing the stack layout here during preparation, we can't rely on downstream stack layout passes doing it for us. =/

256–258 ↗(On Diff #20337)

Range-based for? I think for (const auto &KeyValue : FrameVarInfo) will do the trick.

328–330 ↗(On Diff #20337)

Range for?

347–348 ↗(On Diff #20337)

Ultimately, to reduce register pressure and improve address mode matching, we'll want to sink these GEPs into the basic blocks of their uses. Normally, we could rely on CodeGenPrepare to do that for us, but it runs before EH preparation, so we can't.

We don't have to do that now, but a FIXME would be good. We should be able to write a helper function that iterates the use-list and inserts a GEP prior to each instruction.

605–606 ↗(On Diff #20337)

This is pushing me in the direction of trying to do an upfront analysis that identifies blocks in need of outlining instead of discovering blocks as we go.

test/CodeGen/X86/cppeh-catch-scalar.ll
28 ↗(On Diff #20337)

CHECKL? Is that intentional?

34 ↗(On Diff #20337)

Ditto for CHECT

Thanks for the quick review. I'll clean this up and submit an update tomorrow.

lib/CodeGen/WinEHPrepare.cpp
43 ↗(On Diff #20337)

Sounds good.

245 ↗(On Diff #20337)

Yeah, that definitely needs to happen. I think we'll also need to insert some padding to maintain alignment. I'll add a FIXME.

256–258 ↗(On Diff #20337)

I was avoiding that because the type declaration was so ridiculous. I didn't think about auto.

328–330 ↗(On Diff #20337)

Sure.

347–348 ↗(On Diff #20337)

OK.

605–606 ↗(On Diff #20337)

I have code in my work-in-progress sandbox that does the upfront analysis. I don't believe that handles this case though. I think the problem here is that we've cloned an instruction that uses this variable, but the cloned instruction won't be placed in a BasicBlock until this materialization is done. That shouldn't be difficult to handle, but I didn't want to hold up this review waiting for it.

Implementing review suggestions

rnk accepted this revision.Feb 20 2015, 12:54 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Feb 20 2015, 12:54 PM
majnemer accepted this revision.Feb 20 2015, 5:23 PM
majnemer edited edge metadata.

LGTM

lib/CodeGen/WinEHPrepare.cpp
40 ↗(On Diff #20419)

Please have a space between the Pointee type and the asterisk.

598–599 ↗(On Diff #20419)

This can be folded onto one line.

600–602 ↗(On Diff #20419)

Would it be wrong to call clone on AV? This doesn't handle inalloca correctly.

lib/CodeGen/WinEHPrepare.cpp
600–602 ↗(On Diff #20419)

I'll look into that for the next revision. It sounds like a good idea.

This revision was automatically updated to reflect the committed changes.