llvm dev - Oct 2013 - [LLVMdev] [RFC] Stackmap and Patchpoint Intrinsic Proposal

On 10/22/13 10:48 PM, Andrew Trick wrote:
> I'll respond to a few questions below. I'll start a new thread for
GC
> discussion.
Good idea.  Thanks.
>
> On Oct 22, 2013, at 6:24 PM, Philip R <listmail at philipreames.com 
> <mailto:listmail at philipreames.com>> wrote:
>
>>> Now with regard to patching. I think llvm.patchpoint is generally 
>>> useful for any type of patching I can imagine. It does look like a 
>>> call site in IR, and it’s nice to be able to leverage calling 
>>> conventions to inform the location of arguments.
>> Agreed.  My concern is mostly about naming and documentation of 
>> intended usages. Speaking as someone who's likely to be using this
in
>> the very near future, I'd like to make sure I understand how you 
>> intend it to be used.  The last thing I want to do is misconstrue 
>> your intent and become reliant on a quirk of the implementation you 
>> later want to change.
>
> I don't think the intrinsic names will be able to capture their 
> semantics. I think that's why we need documentation, which I've
been
> working on: http://llvm-reviews.chandlerc.com/D1981.
>
> For example, the "stackmap" intrinsic isn't really a stack
map, it's
> something that allows generation of a stack map in which the entries 
> don't actually need to be on the stack... confusing, but still a good 
> name I think.
"stack map" is also a fairly well understood term in the GC/compiler 
world.  It's better to stick with well known terminology where possible.

As for naming vs documentation, I tend to believe that naming should be 
as descriptive as is reasonable.  Having said that, we're well past the 
point of adding value with this discussion.  Please update the 
documentation to reflect some of the clarifications on usage that have 
come up here and let's move on.
>
>>> But the patchpoint does not have to be a call after patching, and 
>>> you can specify zero arguments to avoid using a calling convention.
>> Er, not quite true.  Your calling convention also influences what 
>> registers stay live across the call.  But in general, I see your point.
>
> You get around that by defining a new calling convention. Each 
> patchpoint intrinsic call can be marked with a different calling 
> convention if you choose. For example, we'll be adding a dynamic 
> calling convention called AnyRegCC. You can use that to effectively 
> specify the number of arguments that you want to force into registers. 
> The stack map will tell you which registers were used for arguments. 
> The "call" will preserves most registers, but clobbers one
register
> (on x86) for use within the code.
Nice trick.  I'll have to remember that.
>
> Another potential extension is to add an entry to the stackmap marking 
> physical registers that are actually in-use across the stack map or 
> patch point.
>
> It helps me to think of llvm.patchpoint as a replacement for any 
> situation where a JIT would have otherwise needed inline asm to 
> generate the desired code sequence.
>
>> (Again, this is touching an area of LLVM I'm not particularly 
>> familiar with.)
>>> In fact, we only currently emit a call out of convenience. We could
>>> splat nops in place and assume the runtime will immediately find
and
>>> patch all occurrences before the code executes. In the future we
may
>>> want to handle NULL call target, bypass call emission, and allow
the
>>> reserved bytes to be less than that required to emit a call.
>> If you were to do that, how would the implementation be different 
>> then the new stackmap intrinsic?  Does that difference imply a 
>> clarification in intended usage or naming?
>
> The implementation of the two intrinsics is actually very similar. In 
> this case, the difference would be that llvm.stackmap does not reserve 
> space for patching, while llvm.patchpoint does.
I'm slightly confused by this given that stackmap takes an argument 
indicating the number of nops to emit as well, but it's not worth 
debating this any more.  Let's move on.  We can revisit this once I'm 
actually using the new intrinsics and can provide real concrete
feedback.
>
> We could have defined different intrinsics for all variations of use 
> cases, but I think two is the right number:
>
> - Use llvm.stackmap if you just want a stack map. No code will be 
> emitted. There is no calling convention. If the runtime patches the 
> code here, it will be destructive.
>
> - Use llvm.patchpoint if you want to reserve space for patching the 
> code. When you do that, you can optionally specify a number of 
> arguments that will follow a specified calling convention. You also 
> get a stack map here because it can be useful to fuse the stack map to 
> the point point location. After all, the runtime needs to know where 
> to patch.
This summary is really helpful.

This summary and the other points you've made in our discussion is 
exactly what should be in the documentation.  It provides the thought 
process behind their design, and the intended usage scenarios.  Can you 
add a section with this meta information?

Yours,
Philip

p.s. Thank you both for taking the time to hash this through.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131023/ef8a1ed4/attachment.html>

On Oct 23, 2013, at 5:27 PM, Philip Reames <listmail at philipreames.com>
wrote:
>> The implementation of the two intrinsics is actually very similar. In
this case, the difference would be that llvm.stackmap does not reserve space for
patching, while llvm.patchpoint does.
> I'm slightly confused by this given that stackmap takes an argument
indicating the number of nops to emit as well, but it's not worth debating
this any more.  Let's move on.  We can revisit this once I'm actually
using the new intrinsics and can provide real concrete feedback.

I want this to be clear when we expose the intrinsics to other developers. I’ve
been making an effort to improve the docs:
http://llvm-reviews.chandlerc.com/D1981. Please let me know where clarification
is needed.

-Andy


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131023/d3cd1bda/attachment.html>

On 10/23/13 5:38 PM, Andrew Trick wrote:
>
> On Oct 23, 2013, at 5:27 PM, Philip Reames <listmail at philipreames.com
> <mailto:listmail at philipreames.com>> wrote:
>
>>> The implementation of the two intrinsics is actually very similar. 
>>> In this case, the difference would be that llvm.stackmap does not 
>>> reserve space for patching, while llvm.patchpoint does.
>> I'm slightly confused by this given that stackmap takes an argument
>> indicating the number of nops to emit as well, but it's not worth 
>> debating this any more.  Let's move on.  We can revisit this once
I'm
>> actually using the new intrinsics and can provide real concrete 
>> feedback.
>
> I want this to be clear when we expose the intrinsics to other 
> developers. I’ve been making an effort to improve the docs: 
> http://llvm-reviews.chandlerc.com/D1981. Please let me know where 
> clarification is needed
I responded in the review.  The only big thing that might be worth 
discussion here is the "full resume" semantics which are mentioned at 
the very end.  This seemed to disagree with our previous discussion.  
Let me know if you're either a) unclear at what I was getting at or b) 
believe the "full resume" semantics were a key part of the intrinsic.
In either case, we should probably hash it out here.

Philip

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131023/f057327a/attachment.html>