<div dir="ltr">Since I'm not sure you're on IRC...<br><br>If we have column information that's just as good as file and line for discriminating. If those are also the same and we have different basic blocks then we need a discriminator. I don't think that anyone is trying to remove discriminators, just if we have column information we can use that too?<div><br></div><div>-eric</div></div><br><div class="gmail_quote">On Fri, Apr 10, 2015 at 10:10 AM Diego Novillo <<a href="mailto:dnovillo@google.com">dnovillo@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Apr 10, 2015 at 12:59 PM, Duncan P. N. Exon Smith<br>
<<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
>> On 2015 Apr 10, at 09:48, Diego Novillo <<a href="mailto:dnovillo@google.com" target="_blank">dnovillo@google.com</a>> wrote:<br>
>><br>
>> On Fri, Apr 10, 2015 at 11:22 AM, Duncan P. N. Exon Smith<br>
>> <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>>> Why? They'll have different line table entries, even in -gmlt. Here's<br>
>>> what `llvm-dwarfdump -debug-dump=line` gives with my patch:<br>
>><br>
>> Because column information cannot be reliably used to distinguish<br>
>> multiple basic blocks on the same line. In this example we are<br>
>> discussing, it can, but in other circumstances it is not possible.<br>
>> The bigger issues are macro expansions and compiler generated builtin<br>
>> expansions (say, memcpy). There are other examples in the original<br>
>> dwarf proposal (<a href="http://wiki.dwarfstd.org/index.php?title=Path_Discriminators" target="_blank">http://wiki.dwarfstd.org/<u></u>index.php?title=Path_<u></u>Discriminators</a>).<br>
>><br>
>> This is why, we don't even try to use columns. The logic in the sample<br>
>> profiler uses line number and discriminator exclusively because of the<br>
>> various limitations that column info has. So, if two distinct basic<br>
>> blocks share the same line number, we make sure that they get<br>
>> different discriminator numbers.<br>
>><br>
>>> Address Line Column File ISA Discriminator Flags<br>
>>> ------------------ ------ ------ ------ --- ------------- -------------<br>
>>> 0x0000000000000000 2 0 1 0 0 is_stmt<br>
>>> 0x0000000000000007 4 9 1 0 0 is_stmt prologue_end<br>
>>> 0x000000000000000b 4 7 1 0 0<br>
>>> 0x0000000000000013 4 21 1 0 0<br>
>>> 0x0000000000000016 4 19 1 0 0<br>
>>> 0x0000000000000019 5 9 1 0 0 is_stmt<br>
>>> 0x000000000000001c 5 7 1 0 0<br>
>>> 0x000000000000001f 7 1 1 0 0 is_stmt<br>
>>> 0x0000000000000021 7 1 1 0 0 is_stmt end_sequence<br>
>>><br>
>>> 0xb is the end of the one basic block, and 0x13 is the start of the next.<br>
>>> I can easily tell them apart ;). Why do you need a discriminator? Without<br>
>>> my patch, AddDiscriminators would set the discriminator to `1` on the 0x13<br>
>>> entry. Why?<br>
>><br>
>> Because of the limitations with column information. The sample profile<br>
>> pass will not even try to use column numbers. If the code came from<br>
>> something like this:<br>
>><br>
>> #define CMP_INC(x, y) if (x > 0) x++; else y--<br>
>><br>
>> int foo(int x, int y) {<br>
>> CMP_INC(x,y);<br>
>> CMP_INC(y,x);<br>
>> }<br>
>><br>
>> Even with column information, you get the two expansions of CMP_INC at column 3.<br>
><br>
> I don't understand. Why wouldn't you use column information when<br>
> it's there?<br>
<br>
Because we can't trust it (like in the macro expansion case).<br>
<br>
> IIUC, then by your logic, you shouldn't even check if they're on the<br>
> same line or have the same filename; you should just always add a<br>
> discriminator.<br>
<br>
No, that would be unnecessary. You only need a discriminator when the<br>
file and line are the same.<br>
<br>
<br>
Diego.<br>
</blockquote></div>