[PATCH] D12168: Improve the determinism of MergeFunctions

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:159
@@ +158,3 @@
+    uint64_t getNumber(GlobalValue* Global) {
+      auto I = GlobalNumbers.insert({Global, NextNumber});
+      if (I.second)
----------------
Use `std::tie` here, it'll make the code easier to read since you can name `first` and `second`: I had to look up `insert`'s API to figure this out :-)

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:460
@@ -418,1 +459,3 @@
+    //assert(FunctionComparator(F, G, GlobalNumbers).compare() == 0 &&
+    //       "The two functions must be equal");
 
----------------
?

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:484
@@ -446,3 +483,3 @@
 int FunctionComparator::cmpAPFloats(const APFloat &L, const APFloat &R) const {
-  if (int Res = cmpNumbers((uint64_t)&L.getSemantics(),
-                           (uint64_t)&R.getSemantics()))
+  // TODO: This correctly handles all extant fltSemantics, because they all have
+  // different precisions. This isn't very robust, however, if new types with
----------------
existant?

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:489
@@ -449,1 +488,3 @@
+  if (int Res = cmpNumbers(APFloat::semanticsPrecision(SL),
+                           APFloat::semanticsPrecision(SR)))
     return Res;
----------------
Why not also compare min/max exponent here? It seems like you could add `bool operator==(const fltSemantics&, const fltSemantics&)` and use that?

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:616
@@ +615,3 @@
+    // for a given input module and host platform.
+    return cmpStrings(SeqL->getRawDataValues(), SeqR->getRawDataValues());
+  }
----------------
I think it would be worth renaming `cmpStrings` to `cmpMem` because that's what `StringRef` does: it ignores null-termination and does `memcmp` based on the declared size.

Could you also add a mergefuncs test that makes sure this is the case: two functions with ConstantDataArray (or vector) which have the same length, same prefix, a null byte, and then different tail. They shouldn't get merged with the current implementation.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:690
@@ -634,1 +689,3 @@
+    // this. This only affects programs which take BlockAddresses and store them
+    // as constants, which is limited to interepreters, etc.
     return cmpNumbers((uint64_t)L, (uint64_t)R);
----------------
Can you `cmpBasicBlocks`?

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:695
@@ +694,3 @@
+    DEBUG(dbgs() << "Looking at valueID " << L->getValueID() << "\n");
+    llvm_unreachable("Constant ValueID not recognized.");
+  }
----------------
I think some MSVC versions are sad if you don't have a `return` here. It should be conservative, so not `0`.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:729
@@ +728,3 @@
+    return cmpNumbers(cast<IntegerType>(TyL)->getBitWidth(),
+                      cast<IntegerType>(TyR)->getBitWidth());
+  case Type::VectorTyID: {
----------------
Is `TyR` also known to be `IntegerType` here? I don't think it is.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:731
@@ +730,3 @@
+  case Type::VectorTyID: {
+    VectorType *VTyL = cast<VectorType>(TyL), *VTyR = cast<VectorType>(TyR);
+    if (int Res = cmpNumbers(VTyL->getNumElements(), VTyR->getNumElements()))
----------------
Same here.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:998
@@ +997,3 @@
+  if (int Res = cmpNumbers(L->getDialect(), R->getDialect()))
+    return Res;
+  llvm_unreachable("InlineAsm blocks were not uniqued.");
----------------
No comparison of `getType` and `getFunctionType`?

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:999
@@ +998,3 @@
+    return Res;
+  llvm_unreachable("InlineAsm blocks were not uniqued.");
+}
----------------
Same on final return for MSVC.


http://reviews.llvm.org/D12168