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

[David Blaikie via llvm-commits]

OK - thanks for the context.

On Wed, Apr 27, 2016 at 4:05 PM, Dehao Chen <danielcdh at gmail.com> wrote:

> 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
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160427/bd2b6df5/attachment.html>