[PATCH] D11721: [ThinLTO] Data structures for holding ThinLTO function index/summary

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Hi Teresa,

I finally managed to find time to finish reading the related RFC. I haven't understand all the use-cases that drove some design decisions (like why `ThinLTOFunctionSummary` and `ThinLTOFunctionInfo` are two separate entities). I'll re-read the RFC tomorrow and I might become more clear (if you have a pointer where it is discussed, I'm interested).

I left some inline comments, hopefully you'll find some useful :)

-

Mehdi


================
Comment at: include/llvm/IR/ThinLTOInfo.h:2
@@ +1,3 @@
+//===-- llvm/ThinLTOInfo.h - ThinLTO Index and Summary ----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
----------------
I agree with Philip, I'm not sure it needs to be "LTO". Now it is not a blocker to me, it shouldn't be a big deal to introduce it here and refactor it later. At least it won't be coupled to anything else.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:25
@@ +24,3 @@
+
+// Function summary information to aid decisions and implementation of
+// importing. This is a separate class from ThinLTOFunctionInfo to
----------------
I'll never say enough how I really enjoyed the job you did with documenting your stuff with RFCs on the mailing list. Now I hope this information (in the RFCs) will be nicely translated in the code as well.

Since you'll introduce a lot of new classes, what about trying to adopt off-hand a format that will result is an nice doxygen? Especially in stuff that goes the public header.

Just a hint to help: we turned on "autobrief" on in LLVM in May, so the first line is interpreted as a the "brief description" unless you add the keyword \brief (then is it the first sentence that is matched).

>  /// Do the thing with the stuff.
>  ///
>  /// This does whatever, using \c param1 for something and returning
>  /// some sort of result.

or

> /// \brief This function does X. It caches some values or something,
> /// and is complicated in some minor way.

(Now there are no strong requirements of sticking with the doxygen syntax that I'm aware of, and it won't prevent your patches from being integrated).


================
Comment at: include/llvm/IR/ThinLTOInfo.h:80
@@ +79,3 @@
+  // True if we are still parsing into this instance.
+  bool Parsing;
+  // Used to flag functions that have local linkage types and need to
----------------
Is seems to me from reading the implementation that unless you call `recordParsedInfo()` with `nullptr` you always have the invariant `Parsing == !FunctionSummary`. 
So it seems that this flag is useless?

================
Comment at: include/llvm/IR/ThinLTOInfo.h:98
@@ +97,3 @@
+    if (FunctionSummary)
+      delete FunctionSummary;
+  }
----------------
This implies that `ThinLTOFunctionInfo` owns its `FunctionSummary`, so is there a reason not to use `std::unique_ptr`?

================
Comment at: include/llvm/IR/ThinLTOInfo.h:102
@@ +101,3 @@
+  // Use the default copy constructor (when creating combined index),
+  // which will cause the same ThinLTOFunctionSummary object to be shared.
+
----------------
This is scary to read with respect to what I see in the destructor, how are you avoiding double deletion?

================
Comment at: include/llvm/IR/ThinLTOInfo.h:132
@@ +131,3 @@
+// COMDAT functions of the same name.
+typedef std::vector<ThinLTOFunctionInfo> ThinLTOFunctionInfoList;
+// Map from function name to corresponding function info structures.
----------------
Can you provide some information about what happens in the case of COMDAT functions? (if you did on the mailing I'd appreciate a pointer to the RFC thread)

================
Comment at: include/llvm/IR/ThinLTOInfo.h:161
@@ +160,3 @@
+ public:
+  ThinLTOFunctionSummaryIndex() { }
+
----------------
Why do you need this declaration?

================
Comment at: include/llvm/IR/ThinLTOInfo.h:164
@@ +163,3 @@
+  // Use default copy constructor. For the StringMaps this will
+  // call std::swap during the assignment.
+
----------------
Not clear to me how a default copy constructor can involve "std::swap"?


================
Comment at: include/llvm/IR/ThinLTOInfo.h:168
@@ +167,3 @@
+  // (e.g. from a native object wrapped bitcode file's symtab).
+  void setFunctionMap(ThinLTOFunctionMap &&FuncMap) {
+    FunctionMap = std::move(FuncMap);
----------------
I believe the usual pattern is to take by value instead of an R-value ref, unless I missed something.

================
Comment at: include/llvm/IR/ThinLTOInfo.h:188
@@ +187,3 @@
+    if (FunctionMap.count(FuncName))
+      FunctionMap.lookup(FuncName).push_back(Info);
+    else
----------------
Since `ThinLTOFunctionInfo` owns the `FunctionSummary`, it sound it shouldn't be copied but moved, you'll run into double free. 
(unless I misunderstood something about the `ThinLTOFunctionInfo` above, in which case forget this comment)

================
Comment at: include/llvm/IR/ThinLTOInfo.h:191
@@ +190,3 @@
+      FunctionMap.insert(std::make_pair(FuncName,
+                                        ThinLTOFunctionInfoList(1, Info)));
+  }
----------------
I don't think you gain anything by doing this test which involves two queries in the map (lookup + insert/update).
What about just doing:
```
FunctionMap.lookup(FuncName).push_back(std::move(Info));
```


================
Comment at: lib/IR/ThinLTOInfo.cpp:24
@@ +23,3 @@
+void ThinLTOFunctionSummaryIndex::mergeFrom(
+    ThinLTOFunctionSummaryIndex *Other) {
+
----------------
I'm just starting to understand `ThinLTOFunctionSummaryIndex` and some "weirdness" associated.
Do you build a `ThinLTOFunctionSummaryIndex` for each module and only then you combine them together using this `mergeFrom()` method?

Looks like a missing abstraction level, some concepts are mixed together, which would explain why you are using a static `NextModuleId` here, which sounds fishy to me. Also this assumes that the `Other` will have an empty ModuleMap, i.e. all the functions come from the same module.
(unless I misunderstood what is going on here)

================
Comment at: lib/IR/ThinLTOInfo.cpp:44
@@ +43,3 @@
+    if (ModPath.empty()) {
+      ModPath = Info.functionSummary()->modulePath();
+      addModulePath(ModPath, NextModuleId);
----------------
It seems to me that this supposed that `ThinLTOFunctionSummary ` has been parsed. If the `ModulePath` field was stored in the `ThinLTOFunctionInfo`, this wouldn't be an issue.
But since I don't fully understand the reason for the separation of these two classes (I'm probably missing a use-case).


http://reviews.llvm.org/D11721