[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:4532
+
+  if (isSupported({TargetOpcode::G_UMIN, {Ty, Ty}})) {
+    // uadd.sat(a, b) -> a + umin(~a, b)
----------------
foad wrote:
> 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?
We use Helper.lowerFMinNumMaxNum directly in AMDGPULegalizerInfo.

I'm specifically thinking we want to switch which expansion is used on AMDGPU based on whether it's scalar or vector, so we would make these legal and then use the helpers in applyMappingImpl


================
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
+
----------------
foad wrote:
> 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?
Use a target without one. I usually use a Tahiti, Fiji, and gfx900 run line


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-sat.mir:254
+    $vgpr0 = COPY %2
+...
----------------
foad wrote:
> 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?
You can add the tests and use -global-isel-abort=0, and they just won't legalize and defer it to a later patch. Handling the vector cases at least should be as easy as adding the instructions to the fewerElementsVector switch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73051