[PATCH] D156853: [AMDGPU] Add IR lowering changes for preloaded kernargs

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

mostly LGTM with some nits



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp:33
+  const GCNSubtarget &ST;
+
+  unsigned NumFreeUserSGPRs;
----------------
Remove blank lines?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp:46
+  void setInitialFreeUserSGPRsCount() {
+    const unsigned MaxUserSGRPs = ST.getMaxNumUserSGPRs();
+    GCNUserSGPRUsageInfo UserSGPRInfo(F, ST);
----------------
Typo SGRPs


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp:54
+                            uint64_t LastExplicitArgOffset) {
+    //  Check if this arguemnt may be loaded into the same register as the
+    //  previous argument.
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp:56
+    //  previous argument.
+    if (!isAligned(Align(4), ArgOffset) && AllocSize < 4)
+      return true;
----------------
isAligned has a weird argument order


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelArguments.cpp:149
+    if (Arg.use_empty()) {
+      InPreloadSequence = false;
       continue;
----------------
don't see why no uses would call for skipping it. You may just account for this when picking a starting position 


================
Comment at: llvm/lib/Target/AMDGPU/GCNSubtarget.h:1188
+  // preloading kernel arguments.
+  bool needsKernargPreloadBC() const {
+    return hasKernargPreload() && !hasGFX940Insts();
----------------
BC isn't obvious to me. Spell out compatibility? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156853