[PATCH] D46699: [ThinLTO] Print module summary index to assembly

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D46699#1111653, @pcc wrote:

> > Although the support in this patch to skip the summary assembly will not handle this, as it only skips a line at a time and won't understand the subsequent lines which don't start with the summary ID.
>
> You could always count parens and stop when you reach 0. That's probably what the end state for skipping should look like as well.


Yeah, let me do that.

> For printing, if you think that would work better for you I can probably tolerate the single line.

Ok, let's keep it on a single line for now then.



================
Comment at: lib/IR/AsmWriter.cpp:2776
+static std::string getLinkagePrintName(GlobalValue::LinkageTypes LT,
+                                       bool AddSpace = true) {
+  std::string Space = AddSpace ? " " : "";
----------------
dexonsmith wrote:
> pcc wrote:
> > tejohnson wrote:
> > > pcc wrote:
> > > > Maybe changing the other callers to do:
> > > > ```if (GV->getLinkage() != GlobalValue::ExternalLinkage)
> > > >    Out << getLinkagePrintName(GV->getLinkage()) << ' ';
> > > > ```
> > > > would be clearer than adding the argument?
> > > I thought about that but felt it was less churn and clearer to handle it in one centralized place. If you much prefer changing the callers, I can do that instead.
> > Can you do that please? I do prefer to avoid mysterious bool arguments wherever possible.
> I agree with Peter that the `bool` argument is mysterious.  We also don't tend to pass around `std::string`s much.
> 
> Here's a third option that should be less boilerplate.  First, in a prep NFC patch, do:
> - Simplify this to `const char *getLinkageName(GlobalValue::LinkageTypes)`.
> - Add a function, `printLinkageWithSpace(raw_ostream&,GlobalValue::LinkageTypes)`.  Its implementation can call `getLinkageName` and add a space, but return early for `ExternalLinkage`.
> - Update current users to call `printLinkageWithSpace`.
> 
> Then in your patch:
> - Change `getLinkageName` to return `"external"` for `ExternalLinkage`.
> - Use `getLinkageName` where you need it.
I like this suggestion - Peter, does that work for you?


Repository:
  rL LLVM

https://reviews.llvm.org/D46699