[LLVMdev] Proposal: add intrinsics for safe division

[Eric Christopher]

Actually I'm proposing that we see what we can do to unify things up.
Nothing more, nothing less.

-eric

On Fri, May 2, 2014 at 12:16 PM, Filip Pizlo <fpizlo at apple.com> wrote:
> 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
>
>