<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 24, 2014 at 12:05 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Oct 24, 2014 at 11:41 AM, Timur Iskhodzhanov <span dir="ltr"><<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">2014-10-23 18:49 GMT-07:00 David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>>:<br>
<div><div>><br>
><br>
> On Thu, Oct 23, 2014 at 6:21 PM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>><br>
> wrote:<br>
>><br>
>> 2014-10-23 18:10 GMT-07:00 David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>>:<br>
>> ><br>
>> ><br>
>> > On Thu, Oct 23, 2014 at 4:46 PM, Timur Iskhodzhanov<br>
>> > <<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Author: timurrrr<br>
>> >> Date: Thu Oct 23 18:46:28 2014<br>
>> >> New Revision: 220536<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=220536&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=220536&view=rev</a><br>
>> >> Log:<br>
>> >> Make getDISubprogram(const Function *F) available in LLVM<br>
>> >><br>
>> >> Reviewed at <a href="http://reviews.llvm.org/D5950" target="_blank">http://reviews.llvm.org/D5950</a><br>
>> >><br>
>> >> Modified:<br>
>> >> llvm/trunk/include/llvm/IR/DebugInfo.h<br>
>> >> llvm/trunk/lib/IR/DebugInfo.cpp<br>
>> >> llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp<br>
>> >><br>
>> >> Modified: llvm/trunk/include/llvm/IR/DebugInfo.h<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=220536&r1=220535&r2=220536&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=220536&r1=220535&r2=220536&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)<br>
>> >> +++ llvm/trunk/include/llvm/IR/DebugInfo.h Thu Oct 23 18:46:28 2014<br>
>> >> @@ -961,6 +961,11 @@ public:<br>
>> >> /// \brief Find subprogram that is enclosing this scope.<br>
>> >> DISubprogram getDISubprogram(const MDNode *Scope);<br>
>> >><br>
>> >> +/// \brief Find debug info for a given function.<br>
>> >> +/// \returns a valid DISubprogram, if found. Otherwise, it returns an<br>
>> >> empty<br>
>> >> +/// DISubprogram.<br>
>> >> +DISubprogram getDISubprogram(const Function *F);<br>
>> >> +<br>
>> >> /// \brief Find underlying composite type.<br>
>> >> DICompositeType getDICompositeType(DIType T);<br>
>> >><br>
>> >><br>
>> >> Modified: llvm/trunk/lib/IR/DebugInfo.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=220536&r1=220535&r2=220536&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=220536&r1=220535&r2=220536&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)<br>
>> >> +++ llvm/trunk/lib/IR/DebugInfo.cpp Thu Oct 23 18:46:28 2014<br>
>> >> @@ -912,6 +912,24 @@ DISubprogram llvm::getDISubprogram(const<br>
>> >> return DISubprogram();<br>
>> >> }<br>
>> >><br>
>> >> +DISubprogram llvm::getDISubprogram(const Function *F) {<br>
>> >> + // We look for the first instr that has a debug annotation leading<br>
>> >> back<br>
>> >> to F.<br>
>> >> + const LLVMContext &Ctx = F->getParent()->getContext();<br>
>> >> + for (auto &BB : *F) {<br>
>> >> + for (auto &Inst : BB.getInstList()) {<br>
>> >> + DebugLoc DLoc = Inst.getDebugLoc();<br>
>> >> + if (DLoc.isUnknown())<br>
>> >> + continue;<br>
>> >> + const MDNode *Scope = DLoc.getScopeNode(Ctx);<br>
>> >> + DISubprogram Subprogram = getDISubprogram(Scope);<br>
>> >> + if (Subprogram.describes(F))<br>
>> >> + return Subprogram;<br>
>> ><br>
>> ><br>
>> > You've changed this in the moving - it used to return on the first<br>
>> > Instruction with DebugInfo, now it keeps searching.<br>
>><br>
>> Yep – we thought the original version was slightly incorrect?<br>
><br>
><br>
> We? (sorry, perhaps I missed some IRC conversation, etc) - this was<br>
> discussed in code review/on IRC when I described the idea to Diego<br>
> originally. What problem were you seeing/thinking of?<br>
<br>
</div></div>Me and David Majnemer.<br>
There's no particular problem we were trying to fix, but we just kind<br>
of thought the function can try a bit harder than it did.<br>
Is this a wrong approach?<br></blockquote></div></div><div><br>There's no need for it to try harder. It's a (recently established) invariant that if a function has debug info (if the function appears in a CU's subprogram list) then all instructions in that function which have debug locs will chain up to that function and nowhere else.<br><br>So as soon as you find a debug loc, either you've found the subprogram or you can stop searching because this function doesn't have debug info (it might have debug locs inlined into it from some debug-having function, but the function itself does not - these debuglocs are kept around so that if the non-debug-having function is later inlined into a debug-having function, that all works properly)</div><div><br>Make sense?<br></div></div></div></div></blockquote><div><br></div><div>This makes sense. I think it was the loop structure which seemed irregular and lead us to believe that something might be wrong.</div><div><br></div><div>I think it's a bit more clear when written like:</div><div><div>DISubprogram llvm::getDISubprogram(const Function *F) {</div><div> // We look for the first instr that has a debug annotation leading back to F.</div><div> for (auto &BB : *F) {</div><div> auto Inst = std::find_if(BB.begin(), BB.end(), [](const Instruction &Inst) {</div><div> return !Inst.getDebugLoc().isUnknown();</div><div> });</div><div> if (Inst != BB.end()) {</div><div> DebugLoc DLoc = Inst->getDebugLoc();</div><div> const MDNode *Scope = DLoc.getScopeNode(F->getParent()->getContext());</div><div> DISubprogram Subprogram = getDISubprogram(Scope);</div><div> return Subprogram.describes(F) ? Subprogram : DISubprogram();</div><div> }</div><div> }</div><div><br></div><div> return DISubprogram();</div><div>}</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
>> > (also, we could change this function to take a reference instead of a<br>
>> > pointer - I was going to make that change when I saw this semantic<br>
>> > change<br>
>> > that caught me by surprise)<br>
>> ><br>
>> >><br>
>> >> + }<br>
>> >> + }<br>
>> >> +<br>
>> >> + return DISubprogram();<br>
>> >> +}<br>
>> >> +<br>
>> >> DICompositeType llvm::getDICompositeType(DIType T) {<br>
>> >> if (T.isCompositeType())<br>
>> >> return DICompositeType(T);<br>
>> >><br>
>> >> Modified: llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp?rev=220536&r1=220535&r2=220536&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp?rev=220536&r1=220535&r2=220536&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp (original)<br>
>> >> +++ llvm/trunk/lib/Transforms/Scalar/SampleProfile.cpp Thu Oct 23<br>
>> >> 18:46:28<br>
>> >> 2014<br>
>> >> @@ -627,29 +627,6 @@ void SampleProfileLoader::propagateWeigh<br>
>> >> }<br>
>> >> }<br>
>> >><br>
>> >> -/// \brief Locate the DISubprogram for F.<br>
>> >> -///<br>
>> >> -/// We look for the first instruction that has a debug annotation<br>
>> >> -/// leading back to \p F.<br>
>> >> -///<br>
>> >> -/// \returns a valid DISubprogram, if found. Otherwise, it returns an<br>
>> >> empty<br>
>> >> -/// DISubprogram.<br>
>> >> -static const DISubprogram getDISubprogram(Function &F, const<br>
>> >> LLVMContext<br>
>> >> &Ctx) {<br>
>> >> - for (auto &BI : F) {<br>
>> >> - BasicBlock *BB = &BI;<br>
>> >> - for (auto &Inst : BB->getInstList()) {<br>
>> >> - DebugLoc DLoc = Inst.getDebugLoc();<br>
>> >> - if (DLoc.isUnknown())<br>
>> >> - continue;<br>
>> >> - const MDNode *Scope = DLoc.getScopeNode(Ctx);<br>
>> >> - DISubprogram Subprogram = getDISubprogram(Scope);<br>
>> >> - return Subprogram.describes(&F) ? Subprogram : DISubprogram();<br>
>> >> - }<br>
>> >> - }<br>
>> >> -<br>
>> >> - return DISubprogram();<br>
>> >> -}<br>
>> >> -<br>
>> >> /// \brief Get the line number for the function header.<br>
>> >> ///<br>
>> >> /// This looks up function \p F in the current compilation unit and<br>
>> >> @@ -662,7 +639,7 @@ static const DISubprogram getDISubprogra<br>
>> >> /// \returns the line number where \p F is defined. If it returns 0,<br>
>> >> /// it means that there is no debug information available for<br>
>> >> \p<br>
>> >> F.<br>
>> >> unsigned SampleProfileLoader::getFunctionLoc(Function &F) {<br>
>> >> - const DISubprogram &S = getDISubprogram(F, *Ctx);<br>
>> >> + DISubprogram S = getDISubprogram(&F);<br>
>> >> if (S.isSubprogram())<br>
>> >> return S.getLineNumber();<br>
>> >><br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>