[PATCH] D12168: Improve the determinism of MergeFunctions

[Jason Koenig via llvm-commits]

jrkoenig added inline comments.

================
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");
 
----------------
jfb wrote:
> ?
GlobalNumbers is not in scope. Moved this assertion to MergeFunctions, where it is in scope

================
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
----------------
jfb wrote:
> existant?
Extant is technically correct, but I changed it to existing :)

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:489
@@ -449,1 +488,3 @@
+  if (int Res = cmpNumbers(APFloat::semanticsPrecision(SL),
+                           APFloat::semanticsPrecision(SR)))
     return Res;
----------------
jfb wrote:
> Why not also compare min/max exponent here? It seems like you could add `bool operator==(const fltSemantics&, const fltSemantics&)` and use that?
I'll add APFloat::semanticsMaxExponent/MinExponent and use them here in another patch, as that will touch another file.

================
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);
----------------
jfb wrote:
> Can you `cmpBasicBlocks`?
No, cmpBasicBlocks will only verify that the instructions are the same, not that the two Block Addresses point to semantically identical code (because we also need to account for successor blocks). A full solution would do something like compare the functions, and if they are the same, compare the index of each basic block within the function in depth first proposal order.

================
Comment at: lib/Transforms/IPO/MergeFunctions.cpp:729
@@ +728,3 @@
+    return cmpNumbers(cast<IntegerType>(TyL)->getBitWidth(),
+                      cast<IntegerType>(TyR)->getBitWidth());
+  case Type::VectorTyID: {
----------------
jfb wrote:
> Is `TyR` also known to be `IntegerType` here? I don't think it is.
>From right above the switch, TyL->getTypeID() == TyR->getTypeID(), otherwise the comparison would have have returned.

================
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.");
----------------
jfb wrote:
> No comparison of `getType` and `getFunctionType`?
Oh, I pulled in all the fields from llvm::InlineAsmKeyType, but the type is also used in the uniquing. Fixed.


http://reviews.llvm.org/D12168