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

[Tobias Edler von Koch via llvm-commits]

tobiasvk added a comment.

Thanks David for the quick review - agree with most of your points, some comments added.


================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:100
@@ +99,3 @@
+
+static const char *MERGED_SUFFIX = "__merged";
+
----------------
majnemer wrote:
> 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.
Yes, I've been thinking that too. I do vaguely recall, though, that some architectures can't handle symbol names with dots in them? Anyone have any better recollection of this? It's certainly fine for Hexagon.

================
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();
+
----------------
majnemer wrote:
> This looks a lot like `haveSameSpecialState` in Instruction.cpp, it would be best to reuse that code.
Yep, except for some important differences:

  - We treat pointer-to-A, pointer-to-B, and the corresponding int type as equivalent
  - An alloca that allocates more space is 'equivalent' to one that allocates less.

MergeFunctions has its own copy of this stuff too.

================
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);
+}
+
----------------
majnemer wrote:
> Why not let global-opt do this?
Merge(Similar)Functions typically runs near the end of the pipeline after GlobalOpt has already run.


http://reviews.llvm.org/D22051