[PATCH] D78659: Add nomerge function attribute to supress tail merge optimization in simplifyCFG

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

This looks pretty close, let's do one more iteration.



================
Comment at: llvm/include/llvm/IR/InstrTypes.h:1720
 
+  /// Determine if the invoke cannot be tail merged.
+  bool cannotMerge() const { return hasFnAttr(Attribute::NoMerge); }
----------------
Let's say "call" instead of "invoke". This method appears in the CallBase class. "Invoke" specifically refers to a call in LLVM IR which has an exceptional edge attached to it.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1305
+    if (const auto *CB1 = dyn_cast<CallBase>(I1))
+      if (CB1->cannotMerge())
+        return Changed;
----------------
It seems inconsistent that we don't apply the same logic to inline asm here. I think you should modify `cannotMerge` to return `isInlineAsm() || hasFnAttr(...NoMerge)`, and then remove the isInlineAsm case below.


================
Comment at: llvm/test/Transforms/SimplifyCFG/nomerge.ll:8
+; CHECK-LABEL: define void @sink
+; CHECK: tail call void @bar()
+; CHECK: tail call void @bar()
----------------
I would prefer retaining CHECK lines for the basic block labels to make it clear that these calls are all in separate blocks.


================
Comment at: llvm/test/Transforms/SimplifyCFG/nomerge.ll:34
+; CHECK: tail call void @bar()
+; CHECK: tail call void @bar()
+; CHECK: tail call void @bar()
----------------
ditto regarding labels


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78659/new/

https://reviews.llvm.org/D78659