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

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.

================
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());
   }
----------------
tejohnson wrote:
> 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.
Agree with Teresa: I believe the LLVM usual coding style is to not add braces unless ambiguity (if/else nested inside an if).
(I agree that it's harder to make a mistake by keeping braces all the time, but that's not the usual LLVM style)

================
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();
+    }
----------------
tejohnson wrote:
> 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"
Agree as well: this is a common pattern in LLVM.


Repository:
  rL LLVM

https://reviews.llvm.org/D21932