This is an archive of the discontinued LLVM Phabricator instance.

Add the llvm.frameallocate and llvm.recoverframeallocation intrinsics
ClosedPublic

Authored by rnk on Dec 2 2014, 6:09 PM.

Details

Summary

These intrinsics allow multiple functions to share a single stack
allocation from one function's call frame. The function with the
allocation may only perform one allocation, and it must be in the entry
block.

Functions accessing the allocation call llvm.recoverframeallocation with
the function whose frame they are accessing and a frame pointer from an
active call frame of that function.

These intrinsics are very difficult to inline correctly, so the
intention is that they be introduced rarely, or at least very late
during EH preparation.

The patch is still a work in progress, but I wanted to send some code
along. It needs more comments, probably some name bikeshedding, and a
change to the inliner.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 16844.Dec 2 2014, 6:09 PM
rnk retitled this revision from to Add the llvm.frameallocate and llvm.recoverframeallocation intrinsics.
rnk updated this object.
rnk added a reviewer: echristo.
rnk added subscribers: andrew.w.kaylor, chandlerc, Unknown Object (MLST).

The implementation looks OK to me. It also looks like this will meet our needs for outlining functions for exception handling. My comments are pretty much documentation and error handling nits.

docs/LangRef.rst
7238 ↗(On Diff #16844)

You should say something about how (or at least where) this "fixed offset" is determined.

7246 ↗(On Diff #16844)

What happens if 'size' is zero?

7263 ↗(On Diff #16844)

The phrase "the stack memory allocation" is a bit ambiguous. You only mean for it to refer to the part of stack memory allocated by this intrinsic, right? So if the function contains other allocas is addition to this intrinsic the descendant functions can't access that alloca (without rolling it into this frame allocation), yes?

7267 ↗(On Diff #16844)

Can the call be anywhere in the entry block? Can there be allocas before or after this call?

7271 ↗(On Diff #16844)

I think it would be helpful to describe the exception handling case here as an example of why this is useful. With everything kept abstract like this it is hard to understand why anyone would want to do this.

lib/CodeGen/MachineFunction.cpp
594 ↗(On Diff #16844)

Will this round zero up to a non-zero value? If not, then the call to setFrameAddressIsTaken may be incorrect. Also, it seems like a good idea to verify here that FrameAllocationSize is zero before it is set by this function.

lib/IR/Verifier.cpp
2584 ↗(On Diff #16844)

Is there a reasonable way to verify that fp is at least a pointer? Looking at your test case made me think that at the very least null should be flagged as an error, and it seems like any constant is likely to be an error.

test/Verifier/frameallocate.ll
5 ↗(On Diff #16844)

Checking for expected behavior if llvm.frameallocate is called with a zero argument would be nice. You should also have a test for a non-constant argument.

33 ↗(On Diff #16844)

The second argument here looks like an error also.

rnk added a comment.Dec 9 2014, 12:27 PM

Thanks! I'll upload the doc tweaks and DCE.

docs/LangRef.rst
7238 ↗(On Diff #16844)

Sure.

7246 ↗(On Diff #16844)

Same thing as with normal allocas, which is apparently: Allocating zero bytes is legal, but the result is undefined.

7263 ↗(On Diff #16844)

Yeah, I should probably say "one stack memory allocation".

7267 ↗(On Diff #16844)

Anywhere. The same is true for normal allocas. Hypothetically you could call another function that recovers the allocation prior to making the allocation, but that seems like obvious UB not worth mentioning.

7271 ↗(On Diff #16844)

I think that should probably go in the ExceptionHandling document, not LangRef. I suppose I can write something up.

lib/CodeGen/MachineFunction.cpp
594 ↗(On Diff #16844)

I don't think it will round zero up.

Actually, FrameAllocationSize is dead, I just forgot to delete it...

lib/IR/Verifier.cpp
2584 ↗(On Diff #16844)

This should already be handled automatically in VerifyIntrinsicType based on the types in Intrinsics.td.

test/Verifier/frameallocate.ll
5 ↗(On Diff #16844)

Sure, non-constant test makes sense. I'm not particularly interested in the zero size case, as long as making it UB is OK.

33 ↗(On Diff #16844)

Passing null here is just UB at runtime, not a verifier error. This code might never be executed, but it should assemble.

rnk updated this revision to Diff 17088.Dec 9 2014, 12:28 PM
  • Tweak docs and delete dead code.
echristo edited edge metadata.Dec 17 2014, 2:01 PM

Few comments, plus some bits inline:

a) Would be nice if the verifier would look and see if the function passed to recover frame allocation actually has a frame allocation intrinisic listed. Otherwise you'll get segfault/assert during assembly time.

b) The documentation appears a bit misleading, what seems to be happening in the code is that you're recording where you put a particular allocation which is different than allocating at a specific fixed address. The allocations are getting merged.

c) some reason to have the symbol in the same function rather than a load of a symbol off to the side? e.g.
.data
.Lallocation_foo
.long -144

Otherwise it seems to be a reasonable way to pass back and forth some specific data for outlining etc via symbol values.

-eric

docs/LangRef.rst
7301 ↗(On Diff #17088)

This doesn't appear to be true? The allocas seem to be merged and after alignment.

include/llvm/CodeGen/ISDOpcodes.h
75 ↗(On Diff #17088)

Needs docs.

include/llvm/Target/Target.td
868 ↗(On Diff #17088)

Might want to comment why both of these are true.

include/llvm/Target/TargetOpcodes.h
121 ↗(On Diff #17088)

Comments.

lib/CodeGen/MachineFunction.cpp
591 ↗(On Diff #17088)

If this is for a specific use you probably want to reference the intrinsic more directly in the name. Right now CreateFrameAllocation(size) sounds like something I'd call in general :)

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5582 ↗(On Diff #17088)

Elaborate?

5605 ↗(On Diff #17088)

Unused.

5606 ↗(On Diff #17088)

Unused.

test/CodeGen/X86/frameallocate.ll
25 ↗(On Diff #17088)

How about a couple tests that have both allocations and/or alignment requirements?

rnk updated this revision to Diff 17459.Dec 18 2014, 11:32 AM
rnk edited edge metadata.
  • comments
docs/LangRef.rst
7301 ↗(On Diff #17088)

This is how I would *like* it to work. It isn't implemented. I'll weaken this to say it *may* be allocated prior to realignment.

include/llvm/CodeGen/ISDOpcodes.h
75 ↗(On Diff #17088)

done

include/llvm/Target/Target.td
868 ↗(On Diff #17088)

done

lib/CodeGen/MachineFunction.cpp
591 ↗(On Diff #17088)

It is named directly after the intrinsic, llvm.frameallocate. Can you propose a more specific name?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5582 ↗(On Diff #17088)

Every SSA value live across mutliple BBs has an associated virtual register except for static allocas. The address of a static alloca is essentially always rematerialized at the point of use by doing arithmetic on the stack pointer. The same could be done for the result of llvm.frameallocate in many cases. As it is, the code will be less optimal than it could be, but that seems OK.

5605 ↗(On Diff #17088)

done

test/CodeGen/X86/frameallocate.ll
25 ↗(On Diff #17088)

I could do that, but it wouldn't work since it's not implemented. :)

sanjoy added a subscriber: sanjoy.Dec 18 2014, 6:02 PM

How is this expected to interact with nounwind? Specifically, can a nounwind eventually invoke recoverframeallocation?

silvas added a subscriber: silvas.Dec 18 2014, 9:26 PM

Sorry I'm totally clueless, but can you explain the motivation for this? Perhaps there has been an llvmdev RFC for this feature yet? (I don't see anything in my email, but with the mailing lists lately I'm not sure if I trust my inbox).

In the past there have been communication failures without a formal RFC/design discussion on llvmdev to make sure everybody is on the same page (e.g. r209007, although admittedly that was a much more intrusive change than this one), so you probably want to shoot a message there to give everyone a chance to take a look at the design.

rnk added a comment.Dec 19 2014, 11:05 AM

How is this expected to interact with nounwind? Specifically, can a nounwind eventually invoke recoverframeallocation?

Yes, all you need is a frame pointer. In the CodeGen/X86/frameallocate.ll test, you can see how this is done with llvm.frameaddress and without throwing any exceptions.

Sorry I'm totally clueless, but can you explain the motivation for this? Perhaps there has been an llvmdev RFC for this feature yet? (I don't see anything in my email, but with the mailing lists lately I'm not sure if I trust my inbox).

The idea came up in the SEH RFC thread here:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078771.html
I called it llvm.eh.set_capture_block / llvm.eh.get_capture_block at the time, but it generalizes into basically "allocate something relative to the FP" and "recover that allocation from this specific FP".

The motivation is that various handlers such as cleanups in MSVC C++ and SEH have to be separate functions, and they need to be able to access some local variables in the parent function frame via the frame pointer. Various schemes have been proposed, but so far frameallocate is the least intrusive on the rest of LLVM.

In the past there have been communication failures without a formal RFC/design discussion on llvmdev to make sure everybody is on the same page (e.g. r209007, although admittedly that was a much more intrusive change than this one), so you probably want to shoot a message there to give everyone a chance to take a look at the design.

I need to recap the original SEH RFC thread, but the design is all out there already.

Does anyone have any high-level feedback on this idea? I think the code is in reasonable shape, and it would be good to get this in and iterate.

chandlerc accepted this revision.Dec 22 2014, 5:49 PM
chandlerc edited edge metadata.
This revision is now accepted and ready to land.Dec 22 2014, 5:49 PM
This revision was automatically updated to reflect the committed changes.