[PATCH] D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 11 11:12:48 PDT 2018
vsk added a comment.
Thanks for working on this @tyb0807, it looks really close :). There are just a few more minor things I'd like addressed.
================
Comment at: test/DebugInfo/debugify-each.ll:1
+; RUN: opt -debugify-each -O1 -S -print-after=called-value-propagation -o - < %s 2> %t
+; FileCheck < %t %s
----------------
At a high level, we don't want to test the debugify logic in this file, because we already have a test for that. Instead, we want to check that -debugify-each is running after each pass.
Let's minimize this test:
- Please remove the checks for what the inserted debug info metadata looks like.
- We just need two functions, e.g "foo" and "bar", which both only contain "ret void".
- Add RUN lines for: "-debugify-each -O{1,2,3}", and "-enable-debugify -debugify-each -sroa -sccp". All of these separate test runs should share the exact same FileCheck "CHECK" commands. Specifically, we expect the module & function debugify passes to run at least twice. So you could write:
```
; Verify that the module & function (check-)debugify passes run at least twice.
; CHECK: CheckModuleDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
; CHECK: CheckModuleDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
```
These should be the only CHECK lines you need.
================
Comment at: tools/opt/Debugify.cpp:44
+ iterator_range<Module::iterator> Functions,
+ std::string Banner) {
// Skip modules with debug info.
----------------
Could you use a StringRef here to avoid copying the std::string?
================
Comment at: tools/opt/Debugify.cpp:56
DenseMap<uint64_t, DIType *> TypeCache;
- auto getCachedDIType = [&](Type *Ty) -> DIType * {
+ auto getCachedDIType = [&] (Type *Ty) -> DIType * {
uint64_t Size =
----------------
Could you run clang-format? There are some whitespace-only changes which complicate the diff.
================
Comment at: tools/opt/Debugify.cpp:129
+ iterator_range<Module::iterator> Functions,
+ std::string Banner,
+ bool Strip) {
----------------
Ditto, please use StringRef.
================
Comment at: tools/opt/Debugify.cpp:160
auto DL = I.getDebugLoc();
- if (DL) {
+ if (DL && MissingLines.size() > (DL.getLine() - 1)) {
MissingLines.reset(DL.getLine() - 1);
----------------
We should only need special handling for "line 0", as that refers to a compiler-generated "ambiguous" location. I.e, `if (DL && DL.getLine() != 0) { reset(...) }`.
We don't need need any other range checks, because an assertion in BitVector should catch out-of-bounds accesses. This means we can omit the "if (DL)" which follows.
================
Comment at: tools/opt/Debugify.cpp:183
(void)to_integer(DVI->getVariable()->getName(), Var, 10);
- assert(Var <= OriginalNumVars && "Unexpected name for DILocalVariable");
+ assert(Var <= MissingVars.size() && "Unexpected name for DILocalVariable");
MissingVars.reset(Var - 1);
----------------
As this change doesn't change any behavior, let's keep the previous version to minimize the diff.
================
Comment at: tools/opt/Debugify.cpp:207
+ if (Function *F = M.getFunction("llvm.dbg.value"))
+ F->eraseFromParent();
+ return true;
----------------
I still don't understand why we need to erase the declaration of llvm.dbg.value() from the module. Shouldn't StripDebugInfo() do this for us? If not, we should just fix StripDebugInfo() in a follow-up patch.
================
Comment at: tools/opt/Debugify.cpp:230
+
+/// FunctionPass for attaching synthetic debug info to everything, used with the
+/// legacy module pass manager.
----------------
Please replace "to everything" with "to instructions within a single function".
================
Comment at: tools/opt/Debugify.cpp:233
+struct DebugifyFunctionPass : public FunctionPass {
+ bool runOnFunction(Function &F) override {
+ return applyDebugifyMetadata(*(F.getParent()),
----------------
Just a nit, but this could be easier to read if it's split up into logically smaller pieces, e.g:
```
Module &M = *F.getParent();
auto FuncIt = F.getIterator();
return apply...(M, make_range(FuncIt, std::next(FuncIt)), ...);
```
================
Comment at: tools/opt/Debugify.cpp:271
+ return checkDebugifyMetadata(*(F.getParent()),
+ make_range(F.getIterator(), std::next(F.getIterator())),
+ "CheckFunctionDebugify: ",
----------------
Same nitpick about simplifying this callsite.
================
Comment at: tools/opt/opt.cpp:267
+ using super = legacy::PassManager;
+ OptCustomPassManager() : super() {}
+
----------------
The implicit default constructor here calls the base constructor. You can just leave this line out -- the compiler does it for you.
Repository:
rL LLVM
https://reviews.llvm.org/D46525
More information about the llvm-commits
mailing list