[PATCH] D154858: [WIP] [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:984
+                                             unsigned value (first operand). It takes hint for scan
+                                             strategy using second operand (0 for `DPP`` and 1 for 
+                                             `iterative approach`.
----------------
Elaborate that it should work if the target doesn't support the mode (e.g. gfx6/7 have no DPP)


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1932-1936
+  Intrinsic<[llvm_i32_ty], [
+    llvm_i32_ty, // llvm value to reduce
+    llvm_i32_ty  // Strategy switch for DPP/Iterative lowering
+    ],
+    [IntrNoMem, IntrConvergent, IntrWillReturn, IntrNoCallback, IntrNoFree, ImmArg<ArgIndex<1>>]>;
----------------
Define an intrinsic class for these to avoid repeating the signautre each time. Also you still should use a type mangled argument instead of hardcoded i32. 


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4068
 
+bool IsScanImplementedUsingDPP(const GCNSubtarget &ST) {
+  // TODO : Set it to true once Scan is implemented using DPP Approach.
----------------
static, start with lowercase


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4079
+// implemenation
+bool IsImmHintDPP(unsigned ScanStratgyImm) {
+  ScanOptions ScanStrategy =
----------------
static, start with lowercase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154858