[PATCH] D21932: [ThinLTO] Perform profile-guided indirect call promotion

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks for the review. Responses below.


================
Comment at: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp:150
@@ +149,3 @@
+    GlobalValueId = VE.getValues().size();
+    if (Index)
+      for (const auto &GUIDSummaryLists : *Index)
----------------
Prazek wrote:
> early exit here will reduce indentation
ok

================
Comment at: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp:151-161
@@ -135,1 +150,13 @@
+    if (Index)
+      for (const auto &GUIDSummaryLists : *Index)
+        // Examine all summaries for this GUID.
+        for (auto &Summary : GUIDSummaryLists.second)
+          if (auto FS = dyn_cast<FunctionSummary>(Summary.get()))
+            // For each call in the function summary, see if the call
+            // is to a GUID (which means it is for an indirect call,
+            // otherwise we would have a Value for it). If so, synthesize
+            // a value id.
+            for (auto &CallEdge : FS->calls())
+              if (CallEdge.first.isGUID())
+                assignValueId(CallEdge.first.getGUID());
   }
----------------
Prazek wrote:
> I think that you should use braces here. It will be more readable and it will be harder to make mistake modifying it.
I've always been asked to remove braces when not necessary, so I will leave this as-is unless other reviewers concur.

================
Comment at: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp:295-297
@@ +294,5 @@
+  unsigned getValueId(GlobalValue::GUID ValGUID) {
+    const auto &VMI = GUIDToValueIdMap.find(ValGUID);
+    assert(VMI != GUIDToValueIdMap.end());
+    return VMI->second;
+  }
----------------
Prazek wrote:
> I guess you could use .at() member here, so you won't have to check if it's there with assert.
True, but for efficiency I prefer to keep the check only in the debug builds. I will add an assertion message though, which should be here.

================
Comment at: llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp:2858-2862
@@ +2857,7 @@
+    for (const auto &GI : valueIds()) {
+      NameVals.push_back(GI.second);
+      NameVals.push_back(GI.first);
+      Stream.EmitRecord(bitc::VST_CODE_COMBINED_ENTRY, NameVals,
+                        GUIDEntryAbbrev);
+      NameVals.clear();
+    }
----------------
Prazek wrote:
> This code seems a little suspicious because you clear the NameVals vector every time you push it.
> You could just create new vector with thouse 2 values and move it to Stream.EmitRecord.
The clear is needed before using it on the subsequent iteration. This is consistent with the rest of BitcodeWriter.cpp, that uses a single SmallVector per routine, clearing after each EmitRecord call.

I believe this is for efficiency (SmallVector is good for small numbers of vals, has efficient push/pop, but is larger and less efficient to allocate). According to http://llvm.org/docs/ProgrammersManual.html#vector:
"std::vector is well loved and respected. It is useful when SmallVector isn’t: when the size of the vector is often large"


Repository:
  rL LLVM

https://reviews.llvm.org/D21932