<div dir="ltr">OK - thanks for the context.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 27, 2016 at 4:05 PM, Dehao Chen <span dir="ltr"><<a href="mailto:danielcdh@gmail.com" target="_blank">danielcdh@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Currently, column number is not used at all when calculating<br>
discriminator, this is because the only customer of discriminator is<br>
sample profile, which is designed to use line_offset+discriminator to<br>
represent source location for efficiency purposes:<br>
* it saves profile space than using line_offset+column_number+discriminator<br>
* more tolerant to source changes (e.g. reuse profile when indentation<br>
changes for the entire function)<br>
* when debug info size becomes an issue, we can always fall back to<br>
omit column numbers in debug info to save space<br>
* we can reuse profiles generated by gcc, which only uses<br>
line_offset+discriminator to represent source location.<br>
<br>
Dehao<br>
<span class="HOEnZb"><font color="#888888"><br>
Dehao<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, Apr 27, 2016 at 3:48 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> Oh, yes, sorry - I know that discriminators are necessary even in the<br>
> presence of column info for precisely that reason. But what I mean is, it<br>
> looks like in your test case we have column info but we are still using<br>
> discriminators, even though we don't have any duplicate locations (they're<br>
> only duplicate lines, but they all have distinct columns). so I'm wondering<br>
> if the discriminator assignment is using columns and if it isn't, why not?<br>
><br>
> On Wed, Apr 27, 2016 at 1:19 PM, Dehao Chen <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>> wrote:<br>
>><br>
>> From your comment in previous patch <a href="http://reviews.llvm.org/D14464" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14464</a>:<br>
>><br>
>> Even with column info, its insufficient if both calls came from the same<br>
>> macro use:<br>
>><br>
>> #define X f(); f()<br>
>> int main() {<br>
>><br>
>> X;<br>
>> }<br>
>><br>
>> Both calls to 'f' will have the same line and column, the location of the<br>
>> X<br>
>> use.<br>
>><br>
>><br>
>> On Wed, Apr 27, 2016 at 11:34 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> wrote:<br>
>> ><br>
>> ><br>
>> > On Tue, Apr 26, 2016 at 1:58 PM, Dehao Chen via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> Unfortunately, we can't because I need to have two consecutive<br>
>> >> instructions that are both from discriminator 1, but different lines<br>
>> >> to trigger this bug. With your proposed change, the code sequence will<br>
>> >> be:<br>
>> >><br>
>> >> line 4 discriminator 0<br>
>> >> line 4 discriminator 1<br>
>> >> line 5 discriminator 0<br>
>> >> line 5 discriminator 1<br>
>> >><br>
>> >> But with my test case, the code sequence will be:<br>
>> >><br>
>> >> line 4 discriminator 0<br>
>> >> line 5 discriminator 0<br>
>> >> line 4 discriminator 1<br>
>> >> line 5 discriminator 1<br>
>> ><br>
>> ><br>
>> > Ah, OK - thanks for the description (might be worth including that in<br>
>> > the<br>
>> > test case, but I'm not sure - up to you)<br>
>> ><br>
>> > Side note: Are we not including the column info when determining when to<br>
>> > add<br>
>> > discriminators? Is there a reason for that (I vaguely recall asking this<br>
>> > before, so I assume we are intentionally disregarding column info for<br>
>> > some<br>
>> > good reason I've forgotten). Because these locations are all unique once<br>
>> > you<br>
>> > include column info (which we are including by default now).<br>
>> ><br>
>> > - Dave<br>
>> ><br>
>> >><br>
>> >><br>
>> >> On Tue, Apr 26, 2016 at 12:46 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> wrote:<br>
>> >> > dblaikie added inline comments.<br>
>> >> ><br>
>> >> > ================<br>
>> >> > Comment at: test/DebugInfo/X86/discriminator2.ll:9<br>
>> >> > @@ +8,3 @@<br>
>> >> > +; #3 void baz() {<br>
>> >> > +; #4 foo/*discriminator 1*/(bar(),<br>
>> >> > +; #5 bar());bar()/*discriminator 1*/;<br>
>> >> > ----------------<br>
>> >> > Would it be simpler/sufficient to do this:<br>
>> >> ><br>
>> >> > void f1();<br>
>> >> > void f2() {<br>
>> >> > f1(); f1();<br>
>> >> > f1(); f1();<br>
>> >> > }<br>
>> >> ><br>
>> >> > (with column info off? Or, to keep column info on, #define CALLS<br>
>> >> > f1();<br>
>> >> > f1() and have the body of the function be:<br>
>> >> > CALLS;<br>
>> >> > CALLS;<br>
>> >> > )<br>
>> >> ><br>
>> >> ><br>
>> >> > <a href="http://reviews.llvm.org/D19522" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19522</a><br>
>> >> ><br>
>> >> ><br>
>> >> ><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> >> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>