[llvm] r287710 - Before sample pgo annotation, do not inline a function that has no debug info. (NFC)

Mon Nov 28 14:47:54 PST 2016

On Mon, Nov 28, 2016 at 2:32 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Mon, Nov 28, 2016 at 2:18 PM Dehao Chen <dehao at google.com> wrote:
>
>> I thought NFC applies to "obvious change to fix corner-case bugs", and
>> apparently I was wrong and I'm sorry about that.
>>
>
> Ah, right - NFC means No Functional Change. We use this to tag pure
> refactoring changes that do not change the observable behavior of the
> component in question. (eg: changing the API of SmallVector might not
> change the observable behavior of LLVM, but it probably still wouldn't be
> tagged NFC, because we tend to unit test ADTs directly - but changing some
> pass so that it still optimizes in the same way, but doesn't, say, build
> some intermediate data structure anymore, would be NFC because at the level
> we test that pass, nothing has changed)
>
>
>> What's the recommend steps to move forward for situations like this?
>> Shall I revert the patch and send out a code review?
>>
>
> Doesn't seem like there's anything needed to be done - just an FYI of
> don't use 'NFC' when semantics/behavior do change. You included what
> appears to be appropriate test coverage, etc, so no worries there.
>

Thanks for the explanation.

>
>
>>
>> On Mon, Nov 28, 2016 at 10:04 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>
>>
>> On Tue, Nov 22, 2016 at 2:59 PM Dehao Chen via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>> Author: dehao
>> Date: Tue Nov 22 16:50:01 2016
>> New Revision: 287710
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=287710&view=rev
>> Log:
>> Before sample pgo annotation, do not inline a function that has no debug
>> info. (NFC)
>>
>>
>> (+1 to Paul's comment - this sounds like a change, has test changes, and
>> is probably not NFC)
>>
>> Also: This sounds like it does the opposite of what we want as a
>> requirement: that the presence or absence of debug info should not change
>> the resulting code.
>>
>> What's the deal?
>>
>>
>> This particular pass requires debug info to make decisions.
>>
>
> Sorry, clearly not my area of understanding/expertise - could you give a
> brief description of why/how that is?
>

Sample profile is represented as a map from Debug Loc to sample count. The
SampleProfileLoader pass reads in the profile, and then traverse the IR to
read its debug loc and try to find a match from the map. If a match is
found, then the count is used to annotate the IR. (The actual process is
more complication, but this is the basic idea).

So this pass requires that all IR has DebugLoc available, otherwise it
cannot find a match in the profile. As a result, we enforce debug loc to be
available before this pass when building with -fprofile-sample-use

>
>
>> This is mandated by pass manager: even with -g0, PM will make sure debug
>> info is available before this pass.
>>
>
> The PM doesn't produce debug info, though - the client building IR (Clang,
> etc) does.
>

Right, it's the PM, but ParseCodeGenArgs (in frontend) that enforces debug
info:

  // If the user requested to use a sample profile for PGO, then the
  // backend will need to track source location information so the profile
  // can be incorporated into the IR.
  if (!Opts.SampleProfileFile.empty())
    NeedLocTracking = true;

>
>> But if user manually removed the debug info, the code should also handle
>> it correctly, which is fixed by this patch.
>>
>> Dehao
>>
>>
>>
>>
>>
>> If there is no debug info in the callee, inlining it will not help
>> annotator. This avoids infinite loop as reported in PR/31119.
>>
>> Added:
>>     llvm/trunk/test/Transforms/SampleProfile/Inputs/nodebug.prof
>>     llvm/trunk/test/Transforms/SampleProfile/nodebug.ll
>> Modified:
>>     llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
>>     llvm/trunk/test/Transforms/SampleProfile/early-inline.ll
>>
>> Modified: llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
>> Transforms/IPO/SampleProfile.cpp?rev=287710&r1=287709&r2=287710&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp (original)
>> +++ llvm/trunk/lib/Transforms/IPO/SampleProfile.cpp Tue Nov 22 16:50:01
>> 2016
>> @@ -651,6 +651,8 @@ bool SampleProfileLoader::inlineHotFunct
>>        InvokeInst *II = dyn_cast<InvokeInst>(I);
>>        Function *CalledFunction =
>>            (CI == nullptr ? II->getCalledFunction() :
>> CI->getCalledFunction());
>> +      if (!CalledFunction || !CalledFunction->getSubprogram())
>> +        continue;
>>        DebugLoc DLoc = I->getDebugLoc();
>>        uint64_t NumSamples = findCalleeFunctionSamples(*I)-
>> >getTotalSamples();
>>        if ((CI && InlineFunction(CI, IFI)) || (II && InlineFunction(II,
>> IFI))) {
>>
>> Added: llvm/trunk/test/Transforms/SampleProfile/Inputs/nodebug.prof
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> Transforms/SampleProfile/Inputs/nodebug.prof?rev=287710&view=auto
>> ============================================================
>> ==================
>> --- llvm/trunk/test/Transforms/SampleProfile/Inputs/nodebug.prof (added)
>> +++ llvm/trunk/test/Transforms/SampleProfile/Inputs/nodebug.prof Tue Nov
>> 22 16:50:01 2016
>> @@ -0,0 +1,2 @@
>> +foo:100:10
>> + 0: bar:10
>>
>> Modified: llvm/trunk/test/Transforms/SampleProfile/early-inline.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> Transforms/SampleProfile/early-inline.ll?rev=287710&r1=
>> 287709&r2=287710&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/test/Transforms/SampleProfile/early-inline.ll (original)
>> +++ llvm/trunk/test/Transforms/SampleProfile/early-inline.ll Tue Nov 22
>> 16:50:01 2016
>> @@ -28,7 +28,7 @@ define void @_Z3foov() #0 personality i8
>>  }
>>
>>  ; Function Attrs: nounwind uwtable
>> -define internal void @_ZL3barv() #1 {
>> +define internal void @_ZL3barv() !dbg !12 {
>>    ret void
>>  }
>>
>> @@ -45,3 +45,4 @@ declare i32 @__gxx_personality_v0(...)
>>  !9 = !DILocation(line: 6, column: 3, scope: !6)
>>  !10 = !DILocation(line: 8, column: 5, scope: !11)
>>  !11 = distinct !DILexicalBlock(scope: !6, file: !1, line: 7, column: 7)
>> +!12 = distinct !DISubprogram(linkageName: "_ZL3barv", scope: !1, line:
>> 20, scopeLine: 20, unit: !0)
>>
>> Added: llvm/trunk/test/Transforms/SampleProfile/nodebug.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> Transforms/SampleProfile/nodebug.ll?rev=287710&view=auto
>> ============================================================
>> ==================
>> --- llvm/trunk/test/Transforms/SampleProfile/nodebug.ll (added)
>> +++ llvm/trunk/test/Transforms/SampleProfile/nodebug.ll Tue Nov 22
>> 16:50:01 2016
>> @@ -0,0 +1,20 @@
>> +; RUN: opt < %s -sample-profile -sample-profile-file=%S/
>> Inputs/nodebug.prof
>> +
>> +define void @foo() !dbg !3 {
>> +  call void @bar(), !dbg !4
>> +  ret void
>> +}
>> +
>> +define void @bar() {
>> +  call void @bar()
>> +  ret void
>> +}
>> +
>> +!llvm.dbg.cu = !{!0}
>> +!llvm.module.flags = !{!2}
>> +
>> +!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1)
>> +!1 = !DIFile(filename: "t", directory: "/tmp/")
>> +!2 = !{i32 2, !"Debug Info Version", i32 3}
>> +!3 = distinct !DISubprogram(name: "a", scope: !1, file: !1, line: 10,
>> unit: !0)
>> +!4 = !DILocation(line: 10, scope: !3)
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161128/91cede2d/attachment.html>