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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some more thoughts.



================
Comment at: llvm/docs/LangRef.rst:14243
+'``llvm.sshl.sat.*``' Intrinsics
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
----------------
This should be
```
'``llvm.sshl.sat.*``' Intrinsics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```


================
Comment at: llvm/docs/LangRef.rst:14262
+The '``llvm.sshl.sat``' family of intrinsic functions perform signed
+saturation left shift on the first argument.
+
----------------
Here and elsewhere: i strongly suspect it should be `s/saturation/saturating/`


================
Comment at: llvm/docs/LangRef.rst:14292
+'``llvm.ushl.sat.*``' Intrinsics
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
----------------
same


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:800
+
+  unsigned Opcode = N->getOpcode();
+
----------------
Assert that we only ever get `ISD::USHLSAT`/`ISD::SSHLSAT` ?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83216