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

[Jay Foad via Phabricator via llvm-commits]

foad marked 4 inline comments as done.
foad 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;
+}
----------------
arsenm wrote:
> 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
Right, this requires more thought. I was hoping to reuse the exisiting isSupported logic without thinking too hard about it.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:4532
+
+  if (isSupported({TargetOpcode::G_UMIN, {Ty, Ty}})) {
+    // uadd.sat(a, b) -> a + umin(~a, b)
----------------
arsenm wrote:
> I think it would also be beneficial to implement this in terms of two helper functions for the two different expansions. That way it's more flexible if a target wants to contextually use different strategies in a custom lowering
Are there precedents for that kind of helper function that the target calls directly, instead of via the standard lower() action?


================
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
+
----------------
arsenm wrote:
> arsenm wrote:
> > This should get an explicit -mcpu. Maybe a runline for with/without i16 instructions
> Really need at least a VI and gfx9+
What sort of run line do I need to disable i16 instructions?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-sat.mir:254
+    $vgpr0 = COPY %2
+...
----------------
arsenm wrote:
> Should add at least a v2s16, v3s16, v4s16, v2i32, and s64 test
That would require G_SADDO (et al) to be legalizable for all those types, which it currently is not. Do I need to tackle that first, before proceeding with this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73051