[LLVMdev] Proposal: add intrinsics for safe division

[Filip Pizlo]

You're proposing a unified intrinsic that allows Philip's call target reasoning and the current "anything goes" semantics.  I strongly dislike unifying the two.  I don't believe that the current semantics are cleanly expressible using Philip's intrinsic.  Specifying patchpoint semantics in a way that implies that the call target is always called breaks inline caches.  Even if you could make those things work, you'd still have to solve other issues, like the types used to pass the call target.

How would a unified intrinsic work for dynamic object access inline caches and virtual call inline caches?  Presumably you'd want to have a special fake call target that means "this can do anything".  You could say that null means "anything" but this feels ugly.  Other than that, my understanding is that LLVM IR currently doesn't allow for any other kind of "this is a call target that is guaranteed to not be analyzable" construct - hypothetically any extern call could be resolved with LTO or something else.

Claiming in the documentation that the point of a patchpoint is that it always calls the call target would be confusing to anyone wanting to implement inline caches using LLVM.  The purpose of inline caches is to change the call target, or to eliminate the call altogether, using post-compilation patch-up.  The semantics of patchpoints should be crystal-clear regarding this aspect of the intrinsic and the fact that this is allowed.

Also, would you expect LLVM phases that perform optimizations using the call target to recover the call target by unwrapping the function-pointer-to-i8* bitcast?  This part is particularly yucky to me.

There's also the question of how not to break WebKit.  Note that you can accomplish this by saying that a null call target means that the call target can do anything, as I mention above - but I don't like this because it seems like you're creating two intrinsics and selecting them by passing null as one of the arguments rather than selecting them by name.

Long story short: creating unified intrinsics is useful if it clarifies reasoning.  In this case a single unified intrinsic doesn't clarify anything.  We're talking about two different semantics - one that means call and another that means "clobber the world" - and so we should keep them separate.

-Phil


On May 2, 2014 at 12:00:40 PM, Eric Christopher (echristo at gmail.com) wrote:

On Fri, May 2, 2014 at 11:58 AM, Philip Reames  
<listmail at philipreames.com> wrote:  
>  
> On 05/02/2014 11:57 AM, Filip Pizlo wrote:  
>  
>  
> On May 2, 2014 at 11:53:25 AM, Eric Christopher (echristo at gmail.com) wrote:  
>  
> On Wed, Apr 30, 2014 at 10:34 PM, Philip Reames  
> <listmail at philipreames.com> wrote:  
>> Andy - If you're not already following this closely, please start. We've  
>> gotten into fairly fundamental questions of what a patchpoint does.  
>>  
>> Filip,  
>>  
>> I think you've hit the nail on the head. What I'm thinking of as being  
>> patchpoints are not what you think they are. Part of that is that I've got  
>> a local change which adds a very similar construction (called  
>> "statepoints"  
>> for the moment), but I was trying to keep that separate. That also  
>> includes  
>> a lot of GC semantics which are not under discussion currently. My  
>> apologies if that experience bled over into this conversation and made  
>> things more confusing.  
>>  
>> I will note that the documentation for patchpoint say explicitly the  
>> following:  
>> "The ‘llvm.experimental.patchpoint.*‘ intrinsics creates a function call  
>> to  
>> the specified <target> and records the location of specified values in the  
>> stack map."  
>>  
>> My reading has always been that a patchpoint *that isn't patched* is  
>> simply  
>> a call with a stackmap associated with it. To my reading, this can (and  
>> did, and does) indicate my proposed usage would be legal.  
>>  
>  
> I like the idea that the target can be assumed to be called. It makes  
> optimization of the call possible, etc. I think it's definitely worth  
> exploring before we lock down the patchpoint intrinsic.  
>  
> I will actively oppose this.  
>  
> I think it sounds like we need to split patchpoint into two cases. I'm  
> going to send a rough proposal for this later today.  
>  

A proposal definitely sounds interesting but if it makes more sense to  
keep it as single intrinsics then that's preferable. We've already got  
two in a kind of weird way with patchpoint and stackmap.  

-eric  


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