[PATCH] D17028: [ThinLTO] Use MD5 hash in function index.

[David Li via llvm-commits]

davidxl added a comment.

Any test on the recorded source file name?


================
Comment at: include/llvm/IR/FunctionInfo.h:151
@@ -151,1 +150,3 @@
+/// Map from function GUID to corresponding function info structures.
+typedef DenseMap<uint64_t, FunctionInfoList> FunctionInfoMapTy;
 
----------------
I am a little concerned with using DenseMap data structure here.

Both DenseMap and StringMap are implemented as open hashtab with quadratic probing. The load factor of both are guaranteed to < 0.75 -- that means there are lots of empty buckets in a large table. The main differences are:

1) StringMap uses indirection -- each bucket contains pointer to the key-value pair object, so the bucket size is small. The copy is also cheaper when rehashing happens. However DenseMap is  key-value pair is embedded inside the bucket. That means if the value type has large size, it will cause lots of waste in memory.  The copy of dense map can also cause large overhead.
2) When resizing, DenseMap will round up the new size to the next power of 2 value -- this further increases the memory overhead.

There is another bad side effect is that if elements are inserted into map one by one, it will incur lots of reallocation operation (just like vector) unless the size of the map is known before hand and properly resized at the beginning.

I suggest using std::map if the size of the map is not known priori. If it is known, and if the map lookup happens after the map is created, it might be better to use a vector (pushback) followed by a sort after the map is populated.

================
Comment at: include/llvm/IR/Module.h:200
@@ -197,1 +199,3 @@
 
+  /// Get the module's original source file name, either the same as the
+  /// ModuleID when compiling from source, or from name recorded in bitcode.
----------------
This comment is not quite clear in its meaning.

================
Comment at: test/tools/gold/X86/thinlto.ll:27
@@ -26,3 +26,3 @@
 ; COMBINED-NEXT: <VALUE_SYMTAB
-; COMBINED-NEXT: <COMBINED_FNENTRY {{.*}} record string = '{{f|g}}'
-; COMBINED-NEXT: <COMBINED_FNENTRY {{.*}} record string = '{{f|g}}'
+; COMBINED-NEXT: <COMBINED_FNENTRY {{.*}}
+; COMBINED-NEXT: <COMBINED_FNENTRY {{.*}}
----------------
test [offset, guid] pattern here?


http://reviews.llvm.org/D17028