[PATCH] D84120: [Debuginfo] (8/N) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).
Jeremy Morse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 30 06:09:37 PDT 2020
jmorse added a comment.
I see what seem to be changes to codegen here (see inline comments), is that intentional? And if so, why is that needed for DW_OP_LLVM_explicit_pointer to work?
================
Comment at: llvm/lib/IR/Metadata.cpp:395
assert(From != To && "Expected changed value");
- assert(From->getType() == To->getType() && "Unexpected type change");
----------------
Why is this now permitted behaviour?
================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:1438-1443
+ auto Changed = CSE.run();
+ if (Changed) {
+ for (BasicBlock &BB : F)
+ RemoveRedundantDbgInstrs(&BB);
+ }
+ return Changed;
----------------
Is this necessary for the implicit pointer work?
================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4366
}
+void SROA::salvageInstruction(Instruction *Ins) {
----------------
Again, this block of code will need comments about its purpose adding.
================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4396-4400
+ const uint64_t *Valptr = Offset.getRawData();
+ if ((AI = findFragmentAlloca(*Valptr))) {
+ Ins->replaceAllUsesWith(AI);
+ DeadInsts.insert(Ins);
+ }
----------------
This part looks like it's changing the code generated, replacing a GEP with an alloca. If that's the case, IMO this should be split out into a different patch (one for codegen, one for debuginfo), as any codegen change will need an independent argument as to why it's a good idea.
Plus, this would only be a codegen change for people building with dwarf-5, which would mean you get different code with and without the -g option to clang.
================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:424
SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
- findDbgUsers(DbgUsers, AI);
+ AI->findDbgUsers(DbgUsers);
for (auto *DII : DbgUsers) {
----------------
This should be folded into the previous patch (7?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84120/new/
https://reviews.llvm.org/D84120
More information about the llvm-commits
mailing list