[PATCH] D91516: [AMDGPU] Replace uses of LDS globals within non-kernel functions by pointers.

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added a comment.

I can't work out which LDS variables you intend to replace with pointers from the code. Could you spell out what the condition under which you intend to replace one is?



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:177
+static bool isUserGlobalVariable(User *U) {
+  SmallVector<User *, 8> UserStack;
+  SmallPtrSet<User *, 8> VisitedUsers;
----------------
Why not isa<GlobalVariable> / function needs a different name


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:204
+// Collect functions whose address is taken within the module.
+static void collectAddressTakenFunctions(
+    CallGraph &CG, SmallPtrSetImpl<CallGraphNode *> &AddressTakenSet) {
----------------
Functions define hasAddressTaken, but also I don't think this pass needs to distinguish between direct and indirect calls


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:224
+
+class ReplaceLDSUseImpl {
+  Module &M;
----------------
This I haven't read yet, but it looks like far too much state. Expected a set of LDS globals called 'toReplaceWithPointer' or similar instead of all the maps


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:384
+      SmallPtrSet<Instruction *, 8> Insts;
+      getInstructions(U, Insts);
+
----------------
Why do we want to replace constexpr with instructions? This comment contradicts the implementation


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUGeneralUtils.cpp:20
+// Check if we can skip the lowering for current LDS global `GV`.
+static bool skipLowering(GlobalVariable *GV,
+                         const SmallPtrSetImpl<GlobalValue *> &UsedList,
----------------
Perhaps name the new files after LDS to make it clearer that they're used for LDS lowering an optimisation, not necessarily general purpose.

Also move the functions out in a separate commit, without changes to their implementation, as that improves the signal/noise of the functional change.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUGeneralUtils.cpp:59
+
+    // `U` should be an instruction belonging to some function.
+    auto *I = dyn_cast<Instruction>(U);
----------------
e.g. I recognise this as newly introduced by the comment, but in phab it's hard to distinguish from things that haven't changed


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUGeneralUtils.h:16
+
+#include "AMDGPU.h"
+#include "Utils/AMDGPUBaseInfo.h"
----------------
Include list should be limited to those that are used by the header, with the ones used by the source included there


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516