[PATCH] D154858: [AMDGPU] Add llvm.amdgcn.wave.reduce.umin/umax Intrinsic.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:996
 
+  llvm.amdgcn.wave.reduce.umin               Performs reduction for minimum operation across the wavefront
+                                             on a given unsigned value (i.e. first operand). 
----------------
unsigned minimum


================
Comment at: llvm/docs/AMDGPUUsage.rst:1005
+
+  llvm.amdgcn.wave.reduce.umax               Performs reduction for maximum operation across the wavefront
+                                             on a given unsigned value (i.e. first operand). 
----------------
unsigned maximum


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1935
+      LLVMMatchType<0>,   // llvm value to reduce (SGPR/VGPR)
+      llvm_i32_ty         // Reduction Strategy Switch for Iterative/DPP lowering (0: Iterative & 1: DPP)
+    ],
----------------
Comment doesn't match the description now


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4067
+static bool isScanImplementedUsingDPP(const GCNSubtarget &ST) {
+  // TODO : Set it to true once Scan is implemented using DPP Approach.
+  bool isScanDPPStrategyImplemented = false;
----------------
No point in this wrapper, whenever the new implementation arrives it will add the check


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4085
+    ScanStrategy = ScanOptions::Iterative;
+    break;
+  case 2:
----------------
Just return true/false?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4090
+  default:
+    llvm_unreachable("Invalid Strategy");
+  }
----------------
Just use the default 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154858