[PATCH] D73051: [GlobalISel][AMDGPU] Saturating add/subtract

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3861-3864
+bool LegalizerHelper::isSupported(const LegalityQuery &Q) {
+  auto QAction = LI.getAction(Q).Action;
+  return QAction == Legal || QAction == Libcall || QAction == Custom;
+}
----------------
I think this won't produce what we want with VOP3P instructions. For v3+s16, we would want G_UMIN to fewerElementsVector to v2s16, which won't be any of these. We would get vector min after lowering


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:4536
+    Register Not =
+        IsAdd ? MIRBuilder.buildNot(Ty, LHS)->getOperand(0).getReg() : LHS;
+    auto Min = MIRBuilder.buildUMin(Ty, Not, RHS);
----------------
You can do .getReg(0) instead of ->getOperand(0).getReg()


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:4549-4550
+    auto Addo = MIRBuilder.buildInstr(Opcode, {Ty, BoolTy}, {LHS, RHS});
+    Register Add = Addo->getOperand(0).getReg();
+    Register Overflow = Addo->getOperand(1).getReg();
+    APInt ClampInt(Ty.getScalarSizeInBits(), IsAdd ? -1 : 0, true);
----------------
Just .getReg(0)/1 works


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:4551-4552
+    Register Overflow = Addo->getOperand(1).getReg();
+    APInt ClampInt(Ty.getScalarSizeInBits(), IsAdd ? -1 : 0, true);
+    auto Clamp = MIRBuilder.buildConstant(Ty, ClampInt);
+    MIRBuilder.buildSelect(Res, Overflow, Clamp, Add);
----------------
Just passing -1/0 to buildConstant should work


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:309
+  getActionDefinitionsBuilder({G_UADDSAT, G_SADDSAT, G_USUBSAT, G_SSUBSAT})
+    .lower();
 
----------------
Bot is wrong here


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-sat.ll:3
+
+; CHECK-LABEL: name: uaddsat_i16
+; CHECK: G_UADDSAT
----------------
Should probably just use update_mir_test_checks on this


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-sat.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=amdgcn -run-pass=legalizer %s -o - | FileCheck %s
+
----------------
This should get an explicit -mcpu. Maybe a runline for with/without i16 instructions


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-sat.mir:254
+    $vgpr0 = COPY %2
+...
----------------
Should add at least a v2s16, v3s16, v4s16, v2i32, and s64 test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73051