[PATCH] D83216: [Intrinsic] Add sshl.sat/ushl.sat, saturated shift intrinsics.

Wed Jul 8 07:52:14 PDT 2020

lebedev.ri added a comment.

In D83216#2139130 <https://reviews.llvm.org/D83216#2139130>, @ebevhan wrote:

> In D83216#2138974 <https://reviews.llvm.org/D83216#2138974>, @lebedev.ri wrote:
>
> > Patch as-is looks good but i'm not sure what's the RFC status here.
> >  If these new intrinsics were already previously proposed as part of some RFC that got accepted,
> >  can you state that in the patch's description? (with link to the thread)
>
>
> I added the links to the threads I mentioned earlier.
>
> Looking back at the full discussion, it doesn't really seem like any real consensus regarding **how** to implement the types was reached, but the prevailing view was that an altogether new IR type was the best approach. I don't think either I or Leonard thought that was the right (or fastest, at least) way to go, though.
>
> The final listing of intrinsics was in http://lists.llvm.org/pipermail/llvm-dev/2018-September/126311.html but the design has diverged a bit from that since then.

i see.

I think this is fine, but just to be safe, may i suggest to do an RFC for these two intrinsics specifically,
just so we're 100% sure everyone is on the same page about them?

> In D83216#2139104 <https://reviews.llvm.org/D83216#2139104>, @lebedev.ri wrote:
> 
>> In D83216#2139098 <https://reviews.llvm.org/D83216#2139098>, @ebevhan wrote:
>>
>> > Fixed review comment and updated summary.
>>
>>
>> (note that updating commit msg does not automatically update description in phab differential)
> 
> 
> I noticed! I was looking for an arc option to do that but couldn't seem to find one.

Sorry, it's just a repeating issue in many reviews :/

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83216/new/

https://reviews.llvm.org/D83216