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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

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)



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:809
+  } else {
+    Op1Promoted = SExtPromotedInteger(Op1);
+    Op2Promoted = ZExtPromotedInteger(Op2);
----------------
ebevhan wrote:
> lebedev.ri wrote:
> > Actually, why do we need to signext, or even zeroext it?
> > As the comment before function notes, we want anyext, we don't care about those new high bits,
> > because we are immediately going to shift them out.
> > 
> > ```
> > ----------------------------------------
> > Name: promote ushl
> >   %r = ushl_sat i8 %x, %y
> >   ret i8 %r
> > =>
> >   %x_wide = zext i8 %x to i16
> >   %y_wide = zext i8 %y to i16
> >   %t0 = shl i16 %x_wide, 8
> >   %t1 = ushl_sat i16 %t0, %y_wide
> >   %t2 = lshr i16 %t1, 8
> >   %r = trunc i16 %t2 to i8
> >   ret i8 %r
> > 
> > Done: 1
> > Transformation seems to be correct!
> > 
> > ----------------------------------------
> > Name: promote sshl
> >   %r = sshl_sat i8 %x, %y
> >   ret i8 %r
> > =>
> >   %x_wide = zext i8 %x to i16
> >   %y_wide = zext i8 %y to i16
> >   %t0 = shl i16 %x_wide, 8
> >   %t1 = sshl_sat i16 %t0, %y_wide
> >   %t2 = ashr i16 %t1, 8
> >   %r = trunc i16 %t2 to i8
> >   ret i8 %r
> > 
> > Done: 1
> > Transformation seems to be correct!
> > 
> > ```
> > 
> > So i think you want
> > ```
> > SDValue Op1Promoted = GetPromotedInteger(Op1);
> > SDValue Op1Promoted = GetPromotedInteger(Op2);
> > unsigned ShiftOp = Opcode == ISD::USHLSAT ? ISD::SRL : ISD::SRA;
> > ```
> > and maybe get rid of `ShiftOp` variable, or sink it closer to use.
> Ah, that's true. I grabbed it from the ADDSUBSAT promotion without thinking. That needs the proper extension due to the min/max expansion, I think.
Err, i was half-right i think. I guess we actually need to zext the shift amount.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83216