<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On May 20, 2015, at 10:38 AM, Sergey Dmitrouk <<a href="mailto:sdmitrouk@accesssoftek.com" class="">sdmitrouk@accesssoftek.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Hi mcrosier, probinson, dblaikie, echristo,<br class=""><br class="">As FastISel groups local values at the top of each block and doesn't attach any<br class="">debug location to them (it's intensionally cleared to prevent jumping back and<br class="">forth in debuggers), the following situation is possible:<br class=""><br class="">```<br class="">BB#0:<br class=""> .loc 1 2 3<br class=""> ...<br class=""><br class="">BB#1:<br class=""> <constant load><br class=""> .loc 1 3 4<br class=""> ...<br class="">```<br class=""><br class="">Here `<constant load>` "accidentally" gets wrong debug location from the<br class="">preceding block, which is wrong.<br class=""></div></blockquote><div><br class=""></div><div>I played with that some time ago and it’s still sitting somewhere on my things-</div><div>to-look-at list. Some notes about that bellow.</div><br class=""><blockquote type="cite" class=""><div class="">This patch adds basic block post-processing for `FastISel`, which will look for<br class="">first instruction with non-null debug location and assign the location to first<br class="">local value (if one exists), producing the following:<br class=""></div></blockquote><div><br class=""></div><div>Some heuristic has already been proposed for this, see:</div><div><a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D14501&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=ESZglNu_SYn_HO2vOS4fgL6ejPSeZIacnchEdEOa78A&s=gYu3aXDLg9Ja8lxYo1JdC7BN0_fW3Y1RdSkW1vD5Jkw&e=" class="">https://llvm.org/bugs/show_bug.cgi?id=14501</a></div><div>I think the patch it refers to is:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120917/151433.html" class="">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120917/151433.html</a></div><div><br class=""></div><div>I think the heuristic was basically the same and echristo rejected it at the time, but</div><div>maybe he changed his mind?</div><div><br class=""></div><div>Coming back to the problem at hand, it is indeed pretty hard to solve at the</div><div>LocalValueMap level. Just playing with it at the time, I tried to emit the constants</div><div>at the insertion point rather than in the LocalValue area at the top at the block and</div><div>then hoist the instructions if the constant gets reused (updating the debug info). This</div><div>doesn’t work because the materialization of a constant might recursively trigger the</div><div>materialization of another one, creating a dependency between the constants. If we</div><div>ever wanted to modify the logic to emit the constants near there first use, they’d need</div><div>to be handled in groups of dependent values.</div><div><br class=""></div><div>Another (simpler) approach that I started to investigate, was to remove the LocalValueMap</div><div>altogether. I started gathering some numbers to back that route, but I got diverted at that</div><div>point and never came back to it. On a full clang build in debug mode, the searches for</div><div>constants in the LocalValueMap had an overall hit rate of approximately 5%.</div><div>This number alone doesn’t mean much, it would need to be compared to the relative cost</div><div>of creating/emitting the instructions. It’s also only representative of one use case on one</div><div>architecture (Other architectures would need to be benchmarked, as well as JITs like the FTL</div><div>use case on x86_64). I still think this could be a viable option, but it requires a lot of</div><div>benchmarking to convince people.</div><div><br class=""></div><div>One thing that isn’t immediately apparent too, is that this bug can lead to some pretty serious</div><div>debug experience issues. If you debug code using libcxx in lldb (don’t know about gdb), you</div><div>might sometimes run into a mis-behaving ‘next’ over a STL function that would fail to next</div><div>over the function and stop somewhere inside it. It tracked some of these back to this</div><div>behavior. Having a block implicitly ‘extend’ its range till the end of the local value area of the</div><div>next block creates a discrepancy between the scope structure described by the DIEs</div><div>(AT_ranges attributes) and the line table. lldb seems to get confused when one line start in</div><div>one block but ends in another. This happens even at O0, because libcxx has a lot of</div><div>‘always_inline’ attributes.</div><div><br class=""></div><div>Fred</div><br class=""><blockquote type="cite" class=""><div class="">```<br class="">BB#0:<br class=""> .loc 1 2 3<br class=""> ...<br class=""><br class="">BB#1:<br class=""> .loc 1 3 4<br class=""> <constant load><br class=""> .loc 1 3 4<br class=""> ...<br class="">```<br class=""><br class="">REPOSITORY<br class=""> rL LLVM<br class=""><br class=""><a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9887&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=ESZglNu_SYn_HO2vOS4fgL6ejPSeZIacnchEdEOa78A&s=0s8L1I1n1h9Z0Tb7fdwheXe-FxuUJfqfW6km0ORNIk4&e=" class="">http://reviews.llvm.org/D9887</a><br class=""><br class="">Files:<br class=""> include/llvm/CodeGen/FastISel.h<br class=""> lib/CodeGen/SelectionDAG/FastISel.cpp<br class=""> lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp<br class=""> test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll<br class=""><br class="">Index: include/llvm/CodeGen/FastISel.h<br class="">===================================================================<br class="">--- include/llvm/CodeGen/FastISel.h<br class="">+++ include/llvm/CodeGen/FastISel.h<br class="">@@ -226,6 +226,9 @@<br class=""> /// be appended, and clear the local CSE map.<br class=""> void startNewBlock();<br class=""><br class="">+ /// \brief Performs post-processing of complete basic block.<br class="">+ void finishNewBlock();<br class="">+<br class=""> /// \brief Return current debug location information.<br class=""> DebugLoc getCurDebugLoc() const { return DbgLoc; }<br class=""><br class="">@@ -569,6 +572,10 @@<br class=""> bool lowerCallOperands(const CallInst *CI, unsigned ArgIdx, unsigned NumArgs,<br class=""> const Value *Callee, bool ForceRetVoidTy,<br class=""> CallLoweringInfo &CLI);<br class="">+<br class="">+ /// \brief Copies first non-empty debug location from list of instructions as<br class="">+ /// starting location for current block.<br class="">+ void fixupLocalValueLocations();<br class=""> };<br class=""><br class=""> } // end namespace llvm<br class="">Index: lib/CodeGen/SelectionDAG/FastISel.cpp<br class="">===================================================================<br class="">--- lib/CodeGen/SelectionDAG/FastISel.cpp<br class="">+++ lib/CodeGen/SelectionDAG/FastISel.cpp<br class="">@@ -104,6 +104,35 @@<br class=""> LastLocalValue = EmitStartPt;<br class=""> }<br class=""><br class="">+void FastISel::finishNewBlock() {<br class="">+ fixupLocalValueLocations();<br class="">+}<br class="">+<br class="">+void FastISel::fixupLocalValueLocations() {<br class="">+ // Nothing to do if we didn't emit any local values.<br class="">+ if (LocalValueMap.empty())<br class="">+ return;<br class="">+<br class="">+ // Skip entry basic block to do not move "prologue_end" location above stack<br class="">+ // adjustment.<br class="">+ if (FuncInfo.MBB == FuncInfo.MBBMap[&FuncInfo.Fn->getEntryBlock()])<br class="">+ return;<br class="">+<br class="">+ // Find first local value by skipping past any EH_LABELs, which go before<br class="">+ // local values.<br class="">+ MachineBasicBlock::iterator FirstLocalValue = FuncInfo.MBB->begin();<br class="">+ while (FirstLocalValue->getOpcode() == TargetOpcode::EH_LABEL)<br class="">+ ++FirstLocalValue;<br class="">+<br class="">+ // Find first instruction with non-null debug location.<br class="">+ MachineBasicBlock::iterator InstWithLoc = FuncInfo.InsertPt;<br class="">+ while (InstWithLoc != FuncInfo.MBB->end() && !InstWithLoc->getDebugLoc())<br class="">+ ++InstWithLoc;<br class="">+<br class="">+ if (InstWithLoc != FuncInfo.MBB->end())<br class="">+ FirstLocalValue->setDebugLoc(InstWithLoc->getDebugLoc());<br class="">+}<br class="">+<br class=""> bool FastISel::lowerArguments() {<br class=""> if (!FuncInfo.CanLowerReturn)<br class=""> // Fallback to SDISel argument lowering code to deal with sret pointer<br class="">Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp<br class="">===================================================================<br class="">--- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp<br class="">+++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp<br class="">@@ -1312,6 +1312,8 @@<br class=""> }<br class=""><br class=""> FinishBasicBlock();<br class="">+ if (FastIS)<br class="">+ FastIS->finishNewBlock();<br class=""> FuncInfo->PHINodesToUpdate.clear();<br class=""> }<br class=""><br class="">Index: test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll<br class="">===================================================================<br class="">--- test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll<br class="">+++ test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll<br class="">@@ -1,4 +1,5 @@<br class=""> ; RUN: llc -filetype=asm -asm-verbose=0 < %s | FileCheck %s<br class="">+; RUN: llc -fast-isel -filetype=asm -asm-verbose=0 < %s | FileCheck %s<br class=""><br class=""> ; int main()<br class=""> ; {<br class=""><br class="">EMAIL PREFERENCES<br class=""> http://reviews.llvm.org/settings/panel/emailpreferences/<br class=""><span id="cid:178FE68D-43A3-4FBF-AFA6-D23B347E9D74@apple.com"><D9887.26161.patch></span>_______________________________________________<br class="">llvm-commits mailing list<br class="">llvm-commits@cs.uiuc.edu<br class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></body></html>