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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
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");
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUSubtarget.h:589
 
+  bool hasNoCarryAddSubSat() const {
+    return GFX9Insts;
----------------
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?


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

https://reviews.llvm.org/D83715