[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/include/llvm/CodeGen/ISDOpcodes.h:313-314
 
+  /// RESULT = [US]SHLSAT(LHS, RHS) - Perform saturation left shift on 2
+  /// integers with the same bit width (W). If the true value of LHS << RHS
+  /// exceeds the largest value that can be represented by W bits, the
----------------
lebedev.ri wrote:
> I'm not sure what `left shift on 2 integers` means.
> Perhaps this needs some rewording.
I lifted it from the other node descriptions, but it doesn't really make sense for shift. I changed it a bit.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7360-7365
+  // For signed shifts, we can check for overflow by checking if we would have
+  // shifted out any bits that disagree with the sign bit. For unsigned shifts,
+  // we can just check if we would have shifted out any ones.
+  // TODO: On targets that don't support CTLZ, it may be more efficient to pull
+  // down the bits to be shifted out and compare those to the signmask/zero
+  // instead.
----------------
lebedev.ri wrote:
> Have you checked if naive `x != ((x << y) u/s>> y)` results in worse lowering?
The CTLZ approach was the one that popped into my head first, so I went with that. But it does turn out that yours works a bit better, at least for sshl.sat, so I swapped it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83216