[PATCH] D34080: [ThinLTO] Add dump-summary command to llvm-lto2 tool

Tue Jun 13 13:56:04 PDT 2017

On Tue, Jun 13, 2017 at 1:27 PM Teresa Johnson via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On Mon, Jun 12, 2017 at 3:08 PM, Chandler Carruth via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Mon, Jun 12, 2017 at 1:50 PM Mehdi AMINI via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> 2017-06-12 13:04 GMT-07:00 Chandler Carruth <chandlerc at gmail.com>:
>>>
>>>> On Mon, Jun 12, 2017 at 12:59 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> On Mon, Jun 12, 2017 at 12:20 PM Peter Collingbourne <peter at pcc.me.uk>
>>>>> wrote:
>>>>>
>>>>>> On Mon, Jun 12, 2017 at 12:16 PM, David Blaikie <dblaikie at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jun 12, 2017 at 12:13 PM Peter Collingbourne <
>>>>>>> peter at pcc.me.uk> wrote:
>>>>>>>
>>>>>>>> I think we should start with this patch so that we aren't forced to
>>>>>>>> go further in what I hope is clearly the wrong direction.
>>>>>>>>
>>>>>>>
>>>>>>> I'm not sure what you mean - it feels to me like this patch is
>>>>>>> "going further in what is clearly the wrong direction" - a direction that's
>>>>>>> adding workarounds by providing dumping features instead of roundtrippable
>>>>>>> .ll syntax?
>>>>>>>
>>>>>>
>>>>>> I don't necessarily agree that this is the wrong direction or a
>>>>>> workaround, which is why I think we should discuss it in parallel.
>>>>>>
>>>>>
>>>>> This has already slipped further than is ideal - when bitcode support
>>>>> was added for summaries, .ll support should've been provided at the same
>>>>> time, as with any other bitcode feature in LLVM. This is a baseline
>>>>> requirement for features in LLVM's bitcode that seems to have been missed
>>>>> during the initial support/code review.
>>>>>
>>>>> When YAML support was added for reading summaries, that would've been
>>>>> a great opportunity to go back and address this missing functionality.
>>>>>
>>>>> Now that dumping support is needed/being added, there's another
>>>>> opportunity to go in that direction rather than continuing to work further
>>>>> away from it.
>>>>>
>>>>
>>>> FWIW, just as textual serialization/de-serialization as part of the
>>>> textual IR was a critical component of preserving use-list order (in
>>>> addition to the bitcode support), I think the same is true for summaries.
>>>>
>>>> I'm perfectly happy to keep them *semantically* distinct from the IR
>>>> (and in a distinct in-memory representation) for now, as that really is a
>>>> separate concern as Mehdi has pointed out in other contexts.
>>>>
>>>> But the basic properties of serialization/de-serialization, being able
>>>> to round trip, and write self-contained test cases should always be common
>>>> between bitcode and textual IR. The fact that summaries have drifted here
>>>> should be corrected sooner rather than later, and insisting upon that
>>>> before going further down the path seems a very reasonable step to ensure
>>>> we get the right thing long-term.
>>>>
>>>
>>> I see two main paths:
>>>
>>> 1) We iron down the right final format for the .ll and implement it. In
>>> the meantime we don't have a good flow to write tests with summaries.
>>> 2) We already have a serialization/deserialization in YAML, why not
>>> using it as a starting point? That was introduced to be able to write
>>> testing without depending on getting the right final format to achieve
>>> `llvm-dis | llvm-as` (i.e. Peter didn't want to hammer down the right .ll
>>> format or wait for it).  We could use this to "bootstrap" by injecting it
>>> in the .ll somehow, and progress as incremental steps from there to a
>>> better .ll format. Knowing that the YAML is machine readable and so can be
>>> upgraded to whatever futur .ll format, so we'll be able to convert to it
>>> automatically and there isn't much debt involved.
>>>
>>> Unless someone wants to drive very actively 1), it seems pragmatic to me
>>> to go with 2) to not block/hinder progress on getting better testing on
>>> LTO/ThinLTO summaries.
>>>
>>
>> My problem is that, as you say, we already unblocked progress without
>> getting #1 done.
>>
>
> I'm not sure we have unblocked progress - without this patch to llvm-lto2
> there isn't a general way to invoke the existing YAML summary dumper (i.e.
> other than from the LowerTypeTests pass and the like). It would be nice to
> get this patch in so we can at access that more easily, given its
> existence.
>

I /think/ Chandler meant that progress has been unblocked a few times now
(when the initial feature was landed without .ll support to match .bc
support, and when the YAML support was added instead of .ll support) &
still the .ll support hasn't manifested.

I think we need to push harder to actually get #1 as folks haven't actually
>> come back to pay down this technical debt even after making short-term
>> progress.
>>
>> (I also agree with David that I don't see any particular reason this
>> would be intractably slow.)
>>
>
> The initial RFC Charles sent a couple weeks back (Subject:
> " [RFC][ThinLTO] llvm-dis ThinLTO summary dump format") has a proposed
> format (not YAML) for the .ll files. Peter had responded that we should
> have a single dumper (which right now is YAML). PTAL at the initial email
> he sent there as that was supposed to be the starting point for a
> discussion about the right long term format for the LLVM assembly.
>

Ah - thanks for bringing this up, it may be better to have the discussion
there (well, except a bunch of it's now here too).

- Dave

>
>
>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170613/61ae75a5/attachment.html>