[PATCH] WIP: AddDiscriminators: Check more carefully for equivalent locations

[Diego Novillo]

On Fri, Apr 10, 2015 at 1:33 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Apr-10, at 10:24, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Thu, Apr 9, 2015 at 5:26 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> Stop using `DILocation::atSameLineAs()` to check whether two locations
>> are equivalent.  This check is too weak: it ignores any column info that
>> might be present, and ignores the scope/inlined-at chains.
>>
>> I don't think we can use the scope/inlined-at can be used here (but, as Eric said on IRC, we might not have testing for it) - we use the scope information to build the scope hierarchy in the debug_info, but it doesn't affect the line table at all. So we could have two debug locations with the same line/file/column but different scopes, yet the same basic block (something as simple as {} can introduce a scope without introducing a new basic block, of course, or a do-while loop would still have a contiguous range but be two basic blocks) and thus they'd coalesce to the same line table entry when they should be separate blocks, potentially.
>>
>> Does that make sense?
>
> Yup, makes sense.  I think where I went wrong was that currently we
> *do* get different line table entries (sounds like that's a bug/missed
> optimization?).
>
> But using column info makes sense, right?  I'll write a FIXME and file
> a PR for that (sounds like maybe you guys need to update tools on your
> end?).

There is an additional tooling problem here, yes.  I don't think that
Linux Perf uses column information when emitting the profile, so we
cannot retrieve them when converting them into LLVM's profile.

If you are removing this test to rely more on column numbers, I think
you'll break users of Linux Perf. Too bad we can't really test in
tree. But, if you must get rid of this, I can look at what needs to be
fixed in the tools outside of LLVM.


Diego.