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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

I have a suggestion for how to respond to Peter's feedback, but this LGTM once he's happy.



================
Comment at: lib/IR/AsmWriter.cpp:2609-2610
+
+  // FIXME: Change AliasSummary to hold a ValueInfo instead of summary pointer
+  // for aliasee (then update BitcodeWriter.cpp and remove get/setAliaseeGUID).
+  for (auto &GlobalList : *TheIndex) {
----------------
tejohnson wrote:
> dexonsmith wrote:
> > - Period at the end of the sentence.
> > - Did you intend to leave this FIXME until later?
> Yes, for now. I'd like to revisit this soon, but wanted to focus on getting the parsing side support done first.
> Can you clarify your concern about the period at the end of the sentence? There's already one there, which I assume is correct.
> Can you clarify your concern about the period at the end of the sentence? There's already one there, which I assume is correct.

Yes it is.  I have no idea why I didn't see it.  Maybe I need to clean my laptop screen :/.


================
Comment at: lib/IR/AsmWriter.cpp:2776
+static std::string getLinkagePrintName(GlobalValue::LinkageTypes LT,
+                                       bool AddSpace = true) {
+  std::string Space = AddSpace ? " " : "";
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D46699