[PATCH] D91516: [AMDGPU] Replace uses of LDS globals within non-kernel functions by pointers.
Jon Chesterfield via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 23 03:42:24 PDT 2021
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
More information about the llvm-commits
mailing list