This is an archive of the discontinued LLVM Phabricator instance.

New StackMap format
Needs ReviewPublic

Authored by kyulee1 on Sep 23 2015, 10:57 AM.

Details

Summary

The current stackmap format exposed limitations. This new is based on the discussed proposal (http://lists.llvm.org/pipermail/llvm-dev/2015-July/087826.html).
In particular, there were missing correlation between functions and stackmap records, which required clients to keep track of the correlations. The new format fixes this issue.
The new format has additional information like function size and call size for the client's needs.
The format is now flat. Every contents are at a fixed offsets from the header, which makes parsing easier.
FrameRegister content has not been implemented yet, which will be reconciled after further clarification.
Note the plan is to keep two versions of stackmap in the current release allowing clients to update their use and then replace the existing version by the new one in the next release.

Diff Detail

Event Timeline

kyulee1 updated this revision to Diff 35528.Sep 23 2015, 10:57 AM
kyulee1 retitled this revision from to New StackMap format.
kyulee1 updated this object.
kyulee1 added reviewers: ributzka, swaroop.sridhar.
kyulee1 added a subscriber: llvm-commits.

Please add -stackmap-version=2 test cases to all backends that support stackmaps. It would be nice to know that, at least, it does not assert somewhere.

docs/StackMaps.rst
468

Are we now guaranteeing that these are sorted by increasing offset?

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
983

Don't need { } here.

lib/CodeGen/StackMaps.cpp
101

Don't need { } here.

reames added a subscriber: reames.Sep 29 2015, 6:10 AM

I've included some larger comments but have not bothered to do a full review. This change will need a lot of work before being ready to land. I strongly suggest separating out the refactorings from this patch to make it easier to review.

include/llvm/CodeGen/StackMaps.h
130

There is no reason to duplicate the entire class just to version the serialization format. Please fix this.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
968

I'm a bit confused, why are these now conditional on whether there's a stackmap?

(p.s. terminology wise we've been using StackMap. Please keep doing so. Changing it everywhere might be fine, changing it piecemeal is not.)

lib/CodeGen/StackMaps.cpp
371

This hook and forward patterns is unacceptably complex. Most of the code is the same for each version and can be shared.

lib/Target/AArch64/AArch64AsmPrinter.cpp
51

This appears to be a refactoring. Please separate into it's own change.

lib/Target/X86/X86MCInstLower.cpp
877

What's this needed for? We already have a label immediately after the call inst.

Also, ordering of the new lines w.r.t. old is confusing.

tools/llvm-readobj/llvm-readobj.cpp
220

The version is part of the file format. This is unnecessary.