[PATCH] D11933: Extending debug ranges
Adrian Prantl via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 26 17:33:38 PDT 2015
aprantl added a comment.
Thanks so far. I added a bunch of more detailed comments inline.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:1
@@ +1,2 @@
+//===-- ExtendDebugRangeLocation.cpp - Fixup Debug Value MIs -----------------------===//
+//
----------------
Ranges are an implementation detail and probably shouldn't show up in the name.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:10
@@ +9,3 @@
+//
+// Fixup DBG_VALUE MachineInstructions
+// a. Fixup debug ranges by inserting new DBG_VALUE instructions at bb join
----------------
This should ideally be a doxygen comment.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:10
@@ +9,3 @@
+//
+// Fixup DBG_VALUE MachineInstructions
+// a. Fixup debug ranges by inserting new DBG_VALUE instructions at bb join
----------------
aprantl wrote:
> This should ideally be a doxygen comment.
Instead of saying "Fixup", better say:
This pass implements a data flow analysis that propagates debug location information by inserting additional DBG_VALUE instructions into the machine instruction stream.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:12
@@ +11,3 @@
+// a. Fixup debug ranges by inserting new DBG_VALUE instructions at bb join
+// points. Current idea is to not build any true ranges here. Only temporary
+// ranges are built to calculate DBG_VALUE at join points. True ranges are
----------------
"This pass internally builds debug location liveness ranges to determine the points where additional DBG_VALUEs need to be inserted."?
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:16
@@ +15,3 @@
+// b. Try to infer the presence of a debug variable at multiple locations from
+// move instructions (TODO).
+// c. Add missing DBG_VALUE instructions, if possible (TODO).
----------------
Please only document what the code does and don't talk about future extensions.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:18
@@ +17,3 @@
+// c. Add missing DBG_VALUE instructions, if possible (TODO).
+// d. Any DBG_VALUE instruction related code.
+//
----------------
???
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:21
@@ +20,3 @@
+// This is a separate pass from DbgValueHistoryCalculator because the
+// MachineFunction(MF) is to be modified (like adding a new DBG_VALUE MI while
+// handling multiple locations etc) and DbgValueHistoryCalculator has a `const'
----------------
This is not a sound argument. If this were implement in DbgValueHistoryCalculator we wouldn't need to emit any DBG_VALUE intrinsics, because we could just pass the ranges themselves to the backend.
Let's just say that this is a separate pass to facilitate testing and improve modularity.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:57
@@ +56,3 @@
+
+ /// \brief Default construct and initialize the pass.
+ ExtendDebugRangeLocation();
----------------
LLVM build with autobrief, so unless you want more than the first sentence to appear in the brief section, the \brief can/should be omitted from the Doxygen comments.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:78
@@ +77,3 @@
+
+ // Member variables and functions for Range Extension across basic blocks
+ // Var location in BB
----------------
///
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:80
@@ +79,3 @@
+ // Var location in BB
+ typedef std::pair<const MachineBasicBlock *, InlinedVariable> VarInBB;
+ // MachineInstr should be a DBG_VALUE instr
----------------
For better readability it might be worth considering using a struct with self-explaining named members instead of an opaque std::pair.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:82
@@ +81,3 @@
+ // MachineInstr should be a DBG_VALUE instr
+ typedef std::pair<VarInBB, const MachineInstr *> VarLoc;
+ typedef std::list<VarLoc> VarLocList;
----------------
Same here.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:139
@@ +138,3 @@
+
+// \brief Return true if @MI1 and @MI2 have equal offsets
+static bool areOffsetsEqual(const MachineInstr &MI1, const MachineInstr &MI2) {
----------------
Full sentences ending with "." are preferred. That said, this comment essentially repeats the function name... what it should say is that it expects DebugValues as inputs.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:159
@@ +158,3 @@
+// redundant ones. The lookup is only till the first non-DBG_VALUE instr.
+// Notify caller by returning the next instruction to start with.
+MachineBasicBlock::iterator
----------------
Better: \return the instruction following \c MI.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:161
@@ +160,3 @@
+MachineBasicBlock::iterator
+ExtendDebugRangeLocation::RemoveRedundantDbgVals(MachineInstr &MI) {
+
----------------
This could be split out into a separate patch and/or tested separately.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:179
@@ +178,3 @@
+ bool Unique = true; // Is the instruction unique? Currently yes.
+ for (auto u = UniqueInstrs.begin(), ue = UniqueInstrs.end(); u != ue; ++u) {
+ MachineInstr *U = *u;
----------------
Could this be written as a ranged-based for?
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:190
@@ +189,3 @@
+ bool OpEqual = true; // Check if all operands other than reg are equal
+ if (! ((i->getNumOperands() == 4) && (U->getNumOperands() == 4)) )
+ OpEqual = false;
----------------
Please run the path through clang-format.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:238
@@ +237,3 @@
+
+ // Finally erase all instructions in EraseInstr
+ for (auto e = EraseInstrs.begin(), ee = EraseInstrs.end(); e != ee; ) {
----------------
missing . at the end
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:239
@@ +238,3 @@
+ // Finally erase all instructions in EraseInstr
+ for (auto e = EraseInstrs.begin(), ee = EraseInstrs.end(); e != ee; ) {
+ auto eCur = e++;
----------------
range-based for?
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:250
@@ +249,3 @@
+ }
+ } // end OutgoingLocs
+ DEBUG(dbgs() << "Removing redundant DBG_VALUE instr: " << *Erase << "\n");
----------------
remove the comment.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:262
@@ +261,3 @@
+
+// Print OpenRanges and OutgoingLocs
+void ExtendDebugRangeLocation::print(const char *msg, raw_ostream &Out) const {
----------------
doxygen
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:280
@@ +279,3 @@
+
+// This is a common transfer function for extending debug ranges and tracking
+// multiple locations for a variable. When HandleMultipleLocations is true, few
----------------
doxygen. (the coding guidelines prefer only a single comment on the declaration, so they don't get out of sync).
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:281
@@ +280,3 @@
+// This is a common transfer function for extending debug ranges and tracking
+// multiple locations for a variable. When HandleMultipleLocations is true, few
+// extra tasks are performed for tracking multiple locations.
----------------
please remove references to multiple locations in this patch
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:289
@@ +288,3 @@
+ // as index into History. The full variables including the
+ // piece expressions are attached to the MI.
+ const DILocalVariable *RawVar = MI.getDebugVariable();
----------------
Why is this correct?
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:306
@@ +305,3 @@
+ // Add Var to OpenRanges from this DBG_VALUE
+ // TODO: Currently handles DBG_VALUE which has only reg as location
+ if (isDescribedByReg(MI))
----------------
Are constants and memory locations handled in a safe/correct way?
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:312
@@ +311,3 @@
+
+ // A definition of a register may mark the end of a range
+ for (const MachineOperand &MO : MI.operands()) {
----------------
The enclosing function is very long. I suggest splitting out certain pieces into separate functions with self-explanatory names so a reader can understand what the function does on a higher level. This looks like a good candidate for that.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:333
@@ +332,3 @@
+ // Early return. Multiple locations need not track OutgoingLocs (code below).
+ if (HandleMultipleLocations)
+ return;
----------------
Please remove anything multiple locations from this patch it makes reviewing more complicated than necessary.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:336
@@ +335,3 @@
+
+ // End all ranges at the end of the current basic block
+ if (MI.isTerminator() || (&MI == &MI.getParent()->instr_back())) {
----------------
Maybe: Terminate all open ranges at the end of the current basic block.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:353
@@ +352,3 @@
+ } // end while
+ } // end terminator instr
+}
----------------
remove these comments.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:356
@@ +355,3 @@
+
+// This routine inserts a new DBG_VALUE instruction at the start of the MBB
+// when the same source variable in all `n' predecessors of MBB reside in the
----------------
Doxygen (see above)
Also the comment should mention that this joins the analysis results of all incoming edges in a basic block.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:359
@@ +358,3 @@
+// same location.
+// TODO: Currently handling when n = {1, 2}. Can mostly do better by having
+// OutgoingLocs in bitvector
----------------
I don't understand the first sentence.
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:393
@@ +392,3 @@
+ for (VarLocList::iterator OLi = OutgoingLocs.begin(),
+ OLe = OutgoingLocs.end(); OLi != OLe; OLi++) {
+ for (VarLocList::iterator OLii = OutgoingLocs.begin(),
----------------
range-based for?
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:404
@@ +403,3 @@
+ (areOffsetsEqual((*OLi->second), (*OLii->second))) ) {
+ // Currently a new range is started for the var from the bb's
+ // beginning by inserting a new DBG_VALUE
----------------
drop "currently"
================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:443
@@ +442,3 @@
+
+ // Perform join() and trasfer() using the worklist until the ranges converge
+ // Ranges have converged when the worklist is empty
----------------
transfer
================
Comment at: test/DebugInfo/MIR/X86/extend-debug-range.mir:101
@@ +100,3 @@
+
+ attributes #0 = { nounwind uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
+ attributes #1 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
We usually drop all non-essential attributes from IR testcases (usually everything in quotes).
http://reviews.llvm.org/D11933
More information about the llvm-commits
mailing list