[PATCH] D83715: AMDGPU/GlobalISel: Use clamp modifier for [us]addsat/[us]subsat

[Matt Arsenault via Phabricator via llvm-commits]

arsenm closed this revision.
arsenm added a comment.

a4edc04693f76eec9068db0556d6533e4c201d74 <https://reviews.llvm.org/rGa4edc04693f76eec9068db0556d6533e4c201d74>



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:424
 
-  if (ST.hasVOP3PInsts()) {
+  if (ST.hasVOP3PInsts() && ST.hasNoCarryAddSubSat()) {
     assert(ST.hasIntClamp() && "all targets with VOP3P should support clamp");
----------------
foad wrote:
> I don't think this change has any effect, does it? This whole hunk already makes loads of assumptions about which features were introduced at the same time as other features.
If you were directly specifying subtarget target features, yes. If you fallback to the base path I would expect everything to work if not be ideal


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:589
 
+  bool hasNoCarryAddSubSat() const {
+    return GFX9Insts;
----------------
foad wrote:
> What does this name mean? My understanding is that gfx9 (a) added add/sub instructions with no carry-out and (b) added "i16" and "i32" add/sub instructions (which let you do signed saturation). But I'm not sure how to parse "NoCarryAddSubSat". Maybe at least add a comment?
I guess I can logically lump all of these with the no carry instructions


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

https://reviews.llvm.org/D83715