[LLVMdev] Proposal: type uniquing of debug info for LTO

[Eric Christopher]

>> Those parts of the codebase actually trying to handle debug info
>> should be handling valid debug info to begin with - we shouldn't need
>> to call "Verify" all over the place & behave differently (at most it
>> should be an assert - but it should probably just move entirely to a
>> verifier like the IR verifier we already have).
>
> Agree.
> The current code base uses Verify to decide what type of DI node we are handling, as given in the above example.
>

Agreed, those cases should be just checking the type explicitly. I
don't think anything needs to be done to the various verify methods
(other than update them slightly for types if we want to verify
subtypes) with the patch, but getting our usage of Verify straightened
out would be general goodness.

Thanks!

-eric

> Thanks,
> Manman
>
>>
>>>
>>>
>>> If you prefer to communicate via lists, let me know.
>>>
>>>
>>> Generally, yes. Discussion should happen in the open unless there's a
>>> need for it to be private.
>>>
>>> As to the MDNode interface, I think debug info is the biggest user, other
>>> usage is quite simple, such as TBAA.
>>>
>>>
>>> I'm not sure what you're referring to here.
>>>
>>>
>>> I am referring to the fact nobody else had comments on our proposal of
>>> modifying MDNode :]
>>> That probably means we are the ones who care the most about MDNode?
>>
>> Probably - but it helps to explain to the community what we're doing &
>> why we're doing it so it's not a surprise when they start seeing
>> commits.
>>
>> - David
>>
>>>
>>> Thanks,
>>> Manman
>>>
>>>
>>>
>>> Thanks,
>>> Manman
>>>
>>> On Jun 21, 2013, at 10:01 PM, Manman Ren wrote:
>>>
>>>
>>> Some scoping for option a:
>>>
>>> There are a few Verify functions
>>> DISubprogram::Verify
>>> DICompositeType::Verify
>>> DIType::Verify
>>> that try to access the context link:
>>> if (getContext() && !getContext().Verify())
>>>  return false;
>>>
>>> And the Verify functions are called from 10+ files.
>>>
>>> If we are oaky with not calling getContext().Verify(), option a) appears
>>> much better to me.
>>> For printing | debugging purpose, we agree that not being able to trace the
>>> pointer is okay.
>>>
>>> Let me know your thoughts,
>>> Manman
>>>
>>> On Jun 21, 2013, at 5:01 PM, Eric Christopher wrote:
>>>
>>> On Fri, Jun 21, 2013 at 4:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> On Fri, Jun 21, 2013 at 4:50 PM, Manman Ren <mren at apple.com> wrote:
>>>
>>>
>>> On Jun 21, 2013, at 4:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> The reason I want a flag is to avoid the need to update the existing testing
>>> cases while this is a work in progress.
>>> I believe migrating one field means updating the existing testing cases?
>>>
>>>
>>> It would, yes - and that's how you'd get coverage to know your change
>>> was stable. You'll have to update all the tests eventually anyway &
>>> doing so incrementally isn't substantially more expensive in my
>>> experience. Perhaps there's something unique to this migration that
>>> would make it so?
>>>
>>>
>>> I am going to update the existing testing cases locally to have the test
>>> coverage, but I don't want to check in the changes often.
>>> Once we turn the flag on by default and remove the other path, I will submit
>>> the changes on the testing cases.
>>>
>>>
>>> I'd rather skip the flag & have the test coverage in the open along
>>> with the changes. Having changes without testing is bad. I don't see a
>>> benefit to you keeping the test coverage locally.
>>>
>>> Another point to have a flag is to check in the patches in steps: DIBuilder
>>> changes, changes related to the map, and DwarfDebug changes.
>>> Without the flag, when I migrate the first field, I have to make sure it
>>> works from frontend all the way to the backend.
>>>
>>>
>>> True enough, but I don't think that's a great drawback.
>>>
>>> Though technically, if you really wanted, you could start at the
>>> backend & make the implementation conditional (check the type of the
>>> field, if it's an MDNode - use that, if it's a string, do the lookup)
>>> - implement that, then port DIBuilder, then remove the conditionality.
>>> Though I don't like it a lot & I think the changes will be small
>>> enough that frontend to backend will be still be understandable
>>> patches.
>>>
>>>
>>> All of this.
>>>
>>> -eric
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>>
>