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

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 12 13:05:37 PDT 2017


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

While I wish I had added some dumping support to the .ll files much
earlier, because it makes testing and analysis much easier, as was pointed
out in the other thread, this is somewhat different than the IR since it is
purely analysis results which summarizes the IR.


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

We already have some YAM dumping support for part of the summaries, making
this more accessible via an internal tool and usable doesn't at all
preclude a parallel discussion about what is the right format for the .ll
files.

>

>
>
>> I hope you can agree in any case that testing for textual attribute
>> names/values is better than testing for magic numbers.
>>
>
> I agree that this is better, but I don't think it's going in the right
> direction and would like to avoid incurring further technical debt without
> much of an idea of how it would be cleaned up.
>

I disagree, we know that there is a desire to get this in the .ll format,
but discussions around what that format should look like may simply take
some time. Adding support to facilitate using the existing YAML support in
the meantime doesn't seem to add much technical debt.

Teresa


> - Dave
>
>
>>
>> Peter
>>
>>>
>>>
>>>> We already have a number of hard to maintain tests that use
>>>> llvm-bcanalyzer output, and I just recently needed to add another one:
>>>> http://llvm-cs.pcc.me.uk/test/Transforms/ThinLTOBitcodeWriter/split.ll#
>>>> 29
>>>>
>>>
>>> Sure - and I'd like those to be fixed in the way that all the other IR
>>> tests tend to work - by using a common .ll representation of what's in .bc
>>> files.
>>>
>>>
>>>> We can decide on the direction in parallel with landing this patch.
>>>>
>>>
>>>> Peter
>>>>
>>>> On Mon, Jun 12, 2017 at 12:05 PM, David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> How does this relate to the other patch, D34063? (trying to understand
>>>>> which pieces do what, etc)
>>>>>
>>>>> That said, I'd really like to go back & have a discussion about
>>>>> whether this is all the right direction, given a desire/need to have .ll
>>>>> consistency with .bc files (ie: a roundtrippable representation of
>>>>> summaries in .ll files).
>>>>>
>>>>> - Dave
>>>>>
>>>>> On Sat, Jun 10, 2017 at 7:47 AM Charles Saternos via Phabricator via
>>>>> llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> ncharlie created this revision.
>>>>>> Herald added subscribers: inglorion, Prazek.
>>>>>>
>>>>>> [ThinLTO] Add dump-summary command to llvm-lto2 tool
>>>>>>
>>>>>> Adds command to dump ThinLTO module summaries in YAML to the
>>>>>> llvm-lto2 tool.
>>>>>>
>>>>>>
>>>>>> https://reviews.llvm.org/D34080
>>>>>>
>>>>>> Files:
>>>>>>   test/tools/llvm-lto2/X86/dump-summary.ll
>>>>>>   tools/llvm-lto2/llvm-lto2.cpp
>>>>>>
>>>>>>
>>>>>> Index: tools/llvm-lto2/llvm-lto2.cpp
>>>>>> ===================================================================
>>>>>> --- tools/llvm-lto2/llvm-lto2.cpp
>>>>>> +++ tools/llvm-lto2/llvm-lto2.cpp
>>>>>> @@ -16,9 +16,11 @@
>>>>>>  //
>>>>>>  //===------------------------------------------------------
>>>>>> ----------------===//
>>>>>>
>>>>>> -#include "llvm/LTO/Caching.h"
>>>>>> +#include "llvm/Bitcode/BitcodeReader.h"
>>>>>>  #include "llvm/CodeGen/CommandFlags.h"
>>>>>>  #include "llvm/IR/DiagnosticPrinter.h"
>>>>>> +#include "llvm/IR/ModuleSummaryIndexYAML.h"
>>>>>> +#include "llvm/LTO/Caching.h"
>>>>>>  #include "llvm/LTO/LTO.h"
>>>>>>  #include "llvm/Support/CommandLine.h"
>>>>>>  #include "llvm/Support/FileSystem.h"
>>>>>> @@ -132,7 +134,7 @@
>>>>>>  }
>>>>>>
>>>>>>  static int usage() {
>>>>>> -  errs() << "Available subcommands: dump-symtab run\n";
>>>>>> +  errs() << "Available subcommands: dump-symtab dump-summary run\n";
>>>>>>    return 1;
>>>>>>  }
>>>>>>
>>>>>> @@ -351,6 +353,19 @@
>>>>>>    return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int dumpSummary(int argc, char **argv) {
>>>>>> +  for (StringRef F : make_range(argv + 1, argv + argc)) {
>>>>>> +    std::unique_ptr<MemoryBuffer> MB = check(MemoryBuffer::getFile(F),
>>>>>> F);
>>>>>> +    std::unique_ptr<ModuleSummaryIndex> Index =
>>>>>> +        check(getModuleSummaryIndex(*MB), F);
>>>>>> +
>>>>>> +    yaml::Output Out(outs());
>>>>>> +    Out << *Index;
>>>>>> +  }
>>>>>> +
>>>>>> +  return 0;
>>>>>> +}
>>>>>> +
>>>>>>  int main(int argc, char **argv) {
>>>>>>    InitializeAllTargets();
>>>>>>    InitializeAllTargetMCs();
>>>>>> @@ -368,6 +383,8 @@
>>>>>>    argv[1] = argv[0];
>>>>>>    if (Subcommand == "dump-symtab")
>>>>>>      return dumpSymtab(argc - 1, argv + 1);
>>>>>> +  if (Subcommand == "dump-summary")
>>>>>> +    return dumpSummary(argc - 1, argv + 1);
>>>>>>    if (Subcommand == "run")
>>>>>>      return run(argc - 1, argv + 1);
>>>>>>    return usage();
>>>>>> Index: test/tools/llvm-lto2/X86/dump-summary.ll
>>>>>> ===================================================================
>>>>>> --- /dev/null
>>>>>> +++ test/tools/llvm-lto2/X86/dump-summary.ll
>>>>>> @@ -0,0 +1,17 @@
>>>>>> +; RUN: opt -module-summary %s -o %t.o
>>>>>> +; RUN: llvm-lto2 dump-summary %t.o | egrep 'Linkage:\s+0'
>>>>>> +; RUN: llvm-lto2 dump-summary %t.o | egrep 'Live:\s+false'
>>>>>> +; RUN: llvm-lto2 dump-summary %t.o | egrep
>>>>>> 'NotEligibleToImport:\s+false'
>>>>>> +
>>>>>> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
>>>>>> +target triple = "x86_64-unknown-linux-gnu"
>>>>>> + at G = constant i32 2048, align 4
>>>>>> + at a = weak alias i32, i32* @G
>>>>>> +
>>>>>> +define void @boop() {
>>>>>> +  tail call void @afun()
>>>>>> +  ret void
>>>>>> +}
>>>>>> +
>>>>>> +declare void @afun()
>>>>>> +declare void @testtest()
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> --
>>>> Peter
>>>>
>>>
>>
>>
>> --
>> --
>> Peter
>>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170612/59838ddb/attachment.html>


More information about the llvm-commits mailing list