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

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan marked 5 inline comments as done.
ebevhan added inline comments.


================
Comment at: llvm/docs/LangRef.rst:14243
+'``llvm.sshl.sat.*``' Intrinsics
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
----------------
lebedev.ri wrote:
> This should be
> ```
> '``llvm.sshl.sat.*``' Intrinsics
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ```
I made the change to the rest of the saturating/fixedpoint intrinsics as well.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:809
+  } else {
+    Op1Promoted = SExtPromotedInteger(Op1);
+    Op2Promoted = ZExtPromotedInteger(Op2);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83216