[PATCH] D83671: GlobalISel: Implement widenScalar for saturating add/sub

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.
This revision is now accepted and ready to land.

Looks fine but can be simplified.



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1644-1645
+                                      LLT WideTy) {
+  bool IsSigned = MI.getOpcode() == TargetOpcode::G_SADDSAT ||
+                  MI.getOpcode() == TargetOpcode::G_SSUBSAT;
+  // We can convert this to:
----------------
This isn't needed if you make the changes below.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1660-1663
+  auto LHS = IsSigned ? MIRBuilder.buildSExt(WideTy, MI.getOperand(1))
+                      : MIRBuilder.buildZExt(WideTy, MI.getOperand(1));
+  auto RHS = IsSigned ? MIRBuilder.buildSExt(WideTy, MI.getOperand(2))
+                      : MIRBuilder.buildZExt(WideTy, MI.getOperand(2));
----------------
These can both be anyext.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1671
+
+  auto Result = IsSigned ? MIRBuilder.buildAShr(WideTy, WideInst, ShiftK)
+                         : MIRBuilder.buildLShr(WideTy, WideInst, ShiftK);
----------------
There's no need to make this conditional on IsSigned if we're just about to truncate anyway.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:1431
+  // FIXME: Placeholder rule. Really depends on whether the clamp modifier is
+  // available, and is selectivly legal for s16, s32, v2s16.
+  getActionDefinitionsBuilder({G_SADDSAT, G_SSUBSAT, G_UADDSAT, G_USUBSAT})
----------------
"selectively"


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

https://reviews.llvm.org/D83671