[PATCH] D19522: Read discriminators correctly from object file.

[Dehao Chen via llvm-commits]

Currently, column number is not used at all when calculating
discriminator, this is because the only customer of discriminator is
sample profile, which is designed to use line_offset+discriminator to
represent source location for efficiency purposes:
* it saves profile space than using line_offset+column_number+discriminator
* more tolerant to source changes (e.g. reuse profile when indentation
changes for the entire function)
* when debug info size becomes an issue, we can always fall back to
omit column numbers in debug info to save space
* we can reuse profiles generated by gcc, which only uses
line_offset+discriminator to represent source location.

Dehao

Dehao

On Wed, Apr 27, 2016 at 3:48 PM, David Blaikie <dblaikie at gmail.com> wrote:
> Oh, yes, sorry - I know that discriminators are necessary even in the
> presence of column info for precisely that reason. But what I mean is, it
> looks like in your test case we have column info but we are still using
> discriminators, even though we don't have any duplicate locations (they're
> only duplicate lines, but they all have distinct columns). so I'm wondering
> if the discriminator assignment is using columns and if it isn't, why not?
>
> On Wed, Apr 27, 2016 at 1:19 PM, Dehao Chen <danielcdh at gmail.com> wrote:
>>
>> From your comment in previous patch http://reviews.llvm.org/D14464:
>>
>> Even with column info, its insufficient if both calls came from the same
>> macro use:
>>
>> #define X f(); f()
>> int main() {
>>
>> X;
>> }
>>
>> Both calls to 'f' will have the same line and column, the location of the
>> X
>> use.
>>
>>
>> On Wed, Apr 27, 2016 at 11:34 AM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >
>> >
>> > On Tue, Apr 26, 2016 at 1:58 PM, Dehao Chen via llvm-commits
>> > <llvm-commits at lists.llvm.org> wrote:
>> >>
>> >> Unfortunately, we can't because I need to have two consecutive
>> >> instructions that are both from discriminator 1, but different lines
>> >> to trigger this bug. With your proposed change, the code sequence will
>> >> be:
>> >>
>> >> line 4 discriminator 0
>> >> line 4 discriminator 1
>> >> line 5 discriminator 0
>> >> line 5 discriminator 1
>> >>
>> >> But with my test case, the code sequence will be:
>> >>
>> >> line 4 discriminator 0
>> >> line 5 discriminator 0
>> >> line 4 discriminator 1
>> >> line 5 discriminator 1
>> >
>> >
>> > Ah, OK - thanks for the description (might be worth including that in
>> > the
>> > test case, but I'm not sure - up to you)
>> >
>> > Side note: Are we not including the column info when determining when to
>> > add
>> > discriminators? Is there a reason for that (I vaguely recall asking this
>> > before, so I assume we are intentionally disregarding column info for
>> > some
>> > good reason I've forgotten). Because these locations are all unique once
>> > you
>> > include column info (which we are including by default now).
>> >
>> > - Dave
>> >
>> >>
>> >>
>> >> On Tue, Apr 26, 2016 at 12:46 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >> > dblaikie added inline comments.
>> >> >
>> >> > ================
>> >> > Comment at: test/DebugInfo/X86/discriminator2.ll:9
>> >> > @@ +8,3 @@
>> >> > +; #3 void baz() {
>> >> > +; #4   foo/*discriminator 1*/(bar(),
>> >> > +; #5       bar());bar()/*discriminator 1*/;
>> >> > ----------------
>> >> > Would it be simpler/sufficient to do this:
>> >> >
>> >> >   void f1();
>> >> >   void f2() {
>> >> >     f1(); f1();
>> >> >     f1(); f1();
>> >> >   }
>> >> >
>> >> > (with column info off? Or, to keep column info on, #define CALLS
>> >> > f1();
>> >> > f1() and have the body of the function be:
>> >> >   CALLS;
>> >> >   CALLS;
>> >> > )
>> >> >
>> >> >
>> >> > http://reviews.llvm.org/D19522
>> >> >
>> >> >
>> >> >
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at lists.llvm.org
>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >
>> >
>
>