[PATCH] D100594: [AMDGPU] Strengthen the logic behind collection of LDS globals for lowering.

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added a comment.

Some comments inline.

If this corrects behaviour as described in the comment, it needs tests. I can't see a behaviour change in the diff.

If it's a non functional refactor, I think it makes the code substantially less legible than it was before. Especially dislike using multiple booleans for control flow.



================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:41
+// llvm.compiler.used globals). All other uses of `GV` can be ignored.
+bool skipLDSLowering(GlobalVariable *GV,
+                     const SmallPtrSetImpl<GlobalValue *> &UsedList) {
----------------
Changing from User to GlobalVariable seems reasonable in itself


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:43
+                     const SmallPtrSetImpl<GlobalValue *> &UsedList) {
+  bool UsedAsGlobalInitializer = false;
+  bool UsedAsNonLLVMUsedInitializer = false;
----------------
Why all these booleans? They make the control flow more complicated, but after staring at it for a while, don't appear to do anything different to the 'continue' we had before.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:50
+
+  // There are no users for `GV`? skip lowering for `GV`.
+  if (UserStack.empty())
----------------
Do you know of prior art for this style of commenting? In general I like comments to describe the 'why', and only the 'what' if the implementation is very complicated. I don't think llvm does much of the `/* this code will do foo */ do_foo()` style.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:125
+  if (GV->getType()->getPointerAddressSpace() != AMDGPUAS::LOCAL_ADDRESS) {
+    // Ignore addrspace other than 3.
+    return false;
----------------
this comment is worse than the equivalent code immediately above since it refers to 3 instead of the named value


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:128
+  }
+
+  if (!GV->hasInitializer()) {
----------------
Moving the predicate (the long sequence of tests) into a separate function seems OK. Should be s/continue/return false/g


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:149
+
+  if (skipLDSLowering(GV, UsedList)) {
+    // We can safely ignore the users of GV, hence lowering of GV is not
----------------
Changing from none of (use requires lowering) to if (can skip) seems ok, not sure if makes any difference


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.h:27
 Align getAlign(DataLayout const &DL, const GlobalVariable *GV);
 
+// Check if we can skip the lowering for current LDS global `GV`.
----------------
I don't think these benefit from comments. Not sure what 'required' can mean in the context of getAlign. Perhaps the clearer thing to do is put the one line functions in the header directly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100594