[PATCH] D22051: MergeSimilarFunctions: a code size pass to merge functions with small differences

[David Majnemer via llvm-commits]

majnemer added a subscriber: majnemer.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:100
@@ +99,3 @@
+
+static const char *MERGED_SUFFIX = "__merged";
+
----------------
This is not a good suffix to use, it is not reserved (user code can have it).

I'd suggest `.merged` because it will demangle nicely and is impossible to get to via most normal means.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:311-324
@@ +310,16 @@
+      && (DstType->isIntegerTy() || DstType->isPointerTy())) {
+    if (InsertBefore)
+      Result = CastInst::CreatePointerCast(V, DstType, "", InsertBefore);
+    else
+      Result = CastInst::CreatePointerCast(V, DstType, "", InsertAtEnd);
+  } else if (OrigType->isIntegerTy() && DstType->isPointerTy()
+             && OrigType == IntPtrType) {
+    // Int -> Ptr
+    if (InsertBefore) {
+      Result = CastInst::Create(CastInst::IntToPtr, V, DstType, "",
+                                InsertBefore);
+    } else {
+      Result = CastInst::Create(CastInst::IntToPtr, V, DstType, "",
+                                InsertAtEnd);
+    }
+  } else {
----------------
This code is a little inconsistent regarding bracing.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:341
@@ +340,3 @@
+public:
+  ComparableFunction() : Func(0), IsNew(false) { }
+
----------------
I'd use an in-class initializer.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:349
@@ +348,3 @@
+
+  ~ComparableFunction() { }
+
----------------
Is this definition/declaration necessary?

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:351-355
@@ +350,7 @@
+
+  ComparableFunction &operator=(const ComparableFunction &that) {
+    Func = that.Func;
+    IsNew = that.IsNew;
+    return *this;
+  }
+
----------------
Would it be insufficient to ` = default;` this operator?

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:376-378
@@ +375,5 @@
+
+}
+
+namespace {
+
----------------
No need to close & reopen the namespace.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:387-388
@@ +386,4 @@
+  FunctionComparator(const DataLayout *DL, Function *F1, Function *F2)
+    : isDifferent(false), isNotMergeable(false),
+      BasicBlockCount(0), InstructionCount(0), DifferingInstructionsCount(0),
+      F1(F1), F2(F2), SimilarityMetric(0), DL(DL), ID(CurID++) {}
----------------
In-class initializer?

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:391
@@ +390,3 @@
+
+  ~FunctionComparator() {}
+
----------------
Remove this?

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:501-503
@@ +500,5 @@
+
+  switch (Ty1->getTypeID()) {
+  default:
+    llvm_unreachable("Unknown type!");
+    // Fall through in Release mode.
----------------
Hmm, this won't handle token type.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:618-645
@@ +617,30 @@
+  }
+  if (const CmpInst *CI = dyn_cast<CmpInst>(I1))
+    return CI->getPredicate() == cast<CmpInst>(I2)->getPredicate();
+  if (const CallInst *CI = dyn_cast<CallInst>(I1))
+    return CI->getCallingConv() == cast<CallInst>(I2)->getCallingConv() &&
+           CI->getAttributes() == cast<CallInst>(I2)->getAttributes();
+  if (const InvokeInst *CI = dyn_cast<InvokeInst>(I1))
+    return CI->getCallingConv() == cast<InvokeInst>(I2)->getCallingConv() &&
+           CI->getAttributes() == cast<InvokeInst>(I2)->getAttributes();
+  if (const InsertValueInst *IVI = dyn_cast<InsertValueInst>(I1))
+    return IVI->getIndices() == cast<InsertValueInst>(I2)->getIndices();
+  if (const ExtractValueInst *EVI = dyn_cast<ExtractValueInst>(I1))
+    return EVI->getIndices() == cast<ExtractValueInst>(I2)->getIndices();
+  if (const FenceInst *FI = dyn_cast<FenceInst>(I1))
+    return FI->getOrdering() == cast<FenceInst>(I2)->getOrdering() &&
+           FI->getSynchScope() == cast<FenceInst>(I2)->getSynchScope();
+  if (const AtomicCmpXchgInst *CXI = dyn_cast<AtomicCmpXchgInst>(I1)) {
+    const AtomicCmpXchgInst *CXI2 = cast<AtomicCmpXchgInst>(I2);
+    return CXI->isVolatile() == CXI2->isVolatile() &&
+           CXI->isWeak() == CXI2->isWeak() &&
+           CXI->getSuccessOrdering() == CXI2->getSuccessOrdering() &&
+           CXI->getFailureOrdering() == CXI2->getFailureOrdering() &&
+           CXI->getSynchScope() == CXI2->getSynchScope();
+  }
+  if (const AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I1))
+    return RMWI->getOperation() == cast<AtomicRMWInst>(I2)->getOperation() &&
+           RMWI->isVolatile() == cast<AtomicRMWInst>(I2)->isVolatile() &&
+           RMWI->getOrdering() == cast<AtomicRMWInst>(I2)->getOrdering() &&
+           RMWI->getSynchScope() == cast<AtomicRMWInst>(I2)->getSynchScope();
+
----------------
This looks a lot like `haveSameSpecialState` in Instruction.cpp, it would be best to reuse that code.

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:1111-1117
@@ +1110,9 @@
+  SimilarFunctions.clear();
+  for (std::list<FunctionComparator *>::iterator
+        I = FunctionsToMerge.begin(), E = FunctionsToMerge.end();
+       I != E; ++I) {
+    FunctionComparator *FC = *I;
+    delete FC;
+  }
+  FunctionsToMerge.clear();
+  FunctionsToCompare.clear();
----------------
Could you use `DeleteContainerPointers` ?

================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:1375-1383
@@ +1374,11 @@
+
+// Replace G with an alias to F if possible, or else a thunk to F. Deletes G.
+void MergeSimilarFunctions::writeThunkOrAlias(Function *F, Function *G) {
+  if (isAliasCapable(G)) {
+    writeAlias(F, G);
+    return;
+  }
+
+  writeThunk(F, G);
+}
+
----------------
Why not let global-opt do this?


http://reviews.llvm.org/D22051