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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:177
+static bool isUserGlobalVariable(User *U) {
+  SmallVector<User *, 8> UserStack;
+  SmallPtrSet<User *, 8> VisitedUsers;
----------------
JonChesterfield wrote:
> Why not isa<GlobalVariable> / function needs a different name
Because LDS would be nested within the const expr within global scope use.


================
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) {
----------------
JonChesterfield wrote:
> Functions define hasAddressTaken, but also I don't think this pass needs to distinguish between direct and indirect calls
I am not getting this comment, probably we can discuss it offline.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:224
+
+class ReplaceLDSUseImpl {
+  Module &M;
----------------
JonChesterfield wrote:
> 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
These maps are required for the logic where we really need to restrict the LDS set for kernel based on kernel excecution paths.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUReplaceLDSUseWithPointer.cpp:384
+      SmallPtrSet<Instruction *, 8> Insts;
+      getInstructions(U, Insts);
+
----------------
JonChesterfield wrote:
> Why do we want to replace constexpr with instructions? This comment contradicts the implementation
Again not clear about what you intended here - let's take it offline.


================
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,
----------------
JonChesterfield wrote:
> 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.
will think about it.


================
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);
----------------
JonChesterfield wrote:
> 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
not sure what you mean here. Let's discuss offline.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516