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

[Manman Ren]

First cleanup patch:
r185020

Let me know if you have questions.

Thanks,
Manman

On Jun 25, 2013, at 3:56 PM, Eric Christopher wrote:

>>> 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
>>>> 
>>>> 
>>>> 
>>