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

Mon Jun 12 12:59:05 PDT 2017

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.

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

- 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170612/8a689afe/attachment.html>