Add nomerge function attribute to supress tail merge optimization in simplifyCFG

[zequanwu]

Summary

We want to add a way to avoid merging identical calls so as to keep the separate debug-information for those calls. There is also an asan usecase where having this attribute would be beneficial to avoid alternative work-arounds.
Here is the link to the feature request: https://bugs.llvm.org/show_bug.cgi?id=42783.
nomerge is different from noline. noinline prevents function from inlining at callsites, but nomerge prevents multiple identical calls from being merged into one.

This patch adds nomerge to disable the optimization in IR level. A followup patch will be needed to let backend understands nomerge and avoid tail merge at backend.

[lebedev.ri]

1. Tests missing
2. Why isn't noinline sufficient, why a whole new attribue?

[lebedev.ri]

+ llvm-commits

[zequanwu]

In D78659#1997528, @lebedev.ri wrote:

1. Tests missing
2. Why isn't noinline sufficient, why a whole new attribue?

1. Test added.
2. We want to avoid merging multiple call sites of same function, but noinline cannot achieve this. For example, we don't want to merge calls in i == 5 and i == 7 cases in the test case.

[asbirlea]

I think that expanding the patch description will help motivate this and clarify the use-cases for which this is useful, and how it differentiates from noinline.
Perhaps include that the attribute aims to avoid merging identical calls so as to keep the separate debug-information for those calls, and that there is an asan usecase where having this attribute would be beneficial to avoid alternative work-arounds.
If you have a link to the feature request for this, include that in the description as well, or include the motivation behind it.

Note there are other LLVM passes (beside SimplifyCFG) that may need to check for this argument to prevent optimizations (e.g. LICM does instruction sinking/hoising, add check ~LICM.cpp:1092?).

Include a comment in the clang/test/CodeGen/attr-nomerge.c test on the expected outcome of the test. Something like:
"The attribute no-merge prevents the 3 bar() calls from being merged into one inside foo. Check that there are still 3 tail calls."

[zequanwu]

Add check in LICM to prevent sink or hoist on nomerge call

[rnk]

In D78659#2003152, @zequanwu wrote:

Add check in LICM to prevent sink or hoist on nomerge call

Does sinking and hoisting remove the debug source location? I assumed that it wouldn't, but now after all the smooth stepping work, maybe it does.

Maybe we should audit for calls to DILocation::getMergedLocation, and use that to guide our changes. It would also be good to build an API that ensures that passes do not use getMergedLocation for instructions marked this way.

---

This also needs to be documented in llvm/docs/LangRef.rst with the other function attributes. Check out Arthur's preallocated patch for an example of that: D74651

clang/include/clang/Basic/AttrDocs.td

356–357

I think we need to expand on this. This is user facing documentation visible here:
http://clang.llvm.org/docs/AttributeReference.html

Here's a suggestion:
"Calls to functions marked `nomerge` will not be merged during optimization. This attribute can be used to prevent the optimizer from obscuring the source location of certain calls. For example, it will prevent tail merging otherwise identical code sequences that raise an exception or terminate the program. Tail merging normally reduces the precision of source location information, making stack traces less useful for debugging. This attribute gives the user control over the tradeoff between code size and debug information precision."

[zequanwu]

Split the the original differential to 2 smaller ones. This one add nomerge function attribute in IR level. Another one will add the attribute to let frontend recognize it.

[zequanwu]

Does sinking and hoisting remove the debug source location? I assumed that it wouldn't, but now after all the smooth stepping work, maybe it does.

Yes, both sinking and hoisting in simplifycfg remove the debug source location by Instruction::applyMergedLocation
sinking: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L1646
hoisting: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L1338

[zequanwu]

Add test case for preventing hoisting.

[rnk]

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

llvm/include/llvm/IR/InstrTypes.h

1720

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.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1292

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.

llvm/test/Transforms/SimplifyCFG/nomerge.ll

8 ↗

(On Diff #260780)

I would prefer retaining CHECK lines for the basic block labels to make it clear that these calls are all in separate blocks.

34 ↗

(On Diff #260780)

ditto regarding labels

[rnk]

I think this looks good, but I would like to get a second opinion from Alina (@asbirlea).

[asbirlea]

Thank you for adding this!

Please update first sentence in the description to:

We want to add a way to avoid merging identical calls so as to keep the separate debug-information for those calls. There is also an asan usecase where having this attribute would be beneficial to avoid alternative work-arounds.

[lebedev.ri]

llvm/include/llvm/IR/Attributes.td

100

Elsewhere we seem to be preventing any kind of movement,
not just disabling inlining of such functions if the call is a tailcall.

[zequanwu]

update comment.

[zequanwu]

add nomerge case in CodeExtractor.cpp

[zequanwu]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1292

This changes breaks the test AArch64/arm64-shrink-wrapping.ll because the inlineAsm cannot be deleted. I think we should only check for hasFnAttr(NoMerge) for CB1 and CB2.

[rnk]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1292

Before pushing this patch, I undid the change to cannotMerge so that this stayed the same and the test passed.

clang/include/clang/Basic/Attr.td

 }
 
 def NoReturn : InheritableAttr {
   let Spellings = [GCC<"noreturn">, Declspec<"noreturn">];
   // FIXME: Does GCC allow this on the function instead?
   let Documentation = [Undocumented];
 }
 
+def NoMerge : InheritableAttr {
+  let Spellings = [Clang<"nomerge">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [NoMergeDocs];
+  let SimpleHandler = 1;
+}
+
 def NoInstrumentFunction : InheritableAttr {
   let Spellings = [GCC<"no_instrument_function">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
 def NotTailCalled : InheritableAttr {

clang/include/clang/Basic/AttrDocs.td

def CXX11NoReturnDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 A function declared as ``[[noreturn]]`` shall not return to its caller. The
 compiler will generate a diagnostic for a function declared as ``[[noreturn]]``
 that appears to be capable of returning to its caller.
   }];
 }
 
+def NoMergeDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+A function declared as ``nomerge`` shall not be tail merged at call sites during
+optimization phase.
+  }];
+}
+
 def AssertCapabilityDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "assert_capability, assert_shared_capability";
   let Content = [{
 Marks a function that dynamically tests whether a capability is held, and halts
 the program if it is not held.
   }];
 }

clang/lib/CodeGen/CGCall.cpp

if (TargetDecl) {
     if (TargetDecl->hasAttr<NoReturnAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::NoReturn);
     if (TargetDecl->hasAttr<ColdAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::Cold);
     if (TargetDecl->hasAttr<NoDuplicateAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::NoDuplicate);
     if (TargetDecl->hasAttr<ConvergentAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::Convergent);
+    if (TargetDecl->hasAttr<NoMergeAttr>())
+      FuncAttrs.addAttribute(llvm::Attribute::NoMerge);
 
     if (const FunctionDecl *Fn = dyn_cast<FunctionDecl>(TargetDecl)) {
       AddAttributesFromFunctionProtoType(
           getContext(), FuncAttrs, Fn->getType()->getAs<FunctionProtoType>());
       if (AttrOnCallSite && Fn->isReplaceableGlobalAllocationFunction()) {
         // A sane operator new returns a non-aliasing pointer.
         auto Kind = Fn->getDeclName().getCXXOverloadedOperator();
         if (getCodeGenOpts().AssumeSaneOperatorNew &&

clang/test/CodeGen/attr-nomerge.c

+// RUN: %clang_cc1 -O2 -S -emit-llvm %s -o - | FileCheck %s
+
+void bar() __attribute__((nomerge));
+
+void foo(int i) {
+    if (i == 5) {
+        bar();
+    }
+    else if (i == 7) {
+        bar();
+    }
+    bar();
+}
+// CHECK: tail call void
+// CHECK: tail call void
+// CHECK: tail call void

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDebug (SubjectMatchRule_type_alias, SubjectMatchRule_hasType_functionType, SubjectMatchRule_objc_method, SubjectMatchRule_variable_not_is_parameter)
 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDuplicate (SubjectMatchRule_function)
 // CHECK-NEXT: NoEscape (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NoInline (SubjectMatchRule_function)
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function)
+// CHECK-NEXT: NoMerge (SubjectMatchRule_function)
 // CHECK-NEXT: NoMicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
 // CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSanitizeSpecific (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: NoSplitStack (SubjectMatchRule_function)
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)

llvm/include/llvm/IR/Attributes.td

 def NoImplicitFloat : EnumAttr<"noimplicitfloat">;
 
 /// inline=never.
 def NoInline : EnumAttr<"noinline">;
 
 /// Function is called early and/or often, so lazy binding isn't worthwhile.
 def NonLazyBind : EnumAttr<"nonlazybind">;
 
+/// Disable tail merge for this function
+def NoMerge : EnumAttr<"nomerge">;
+
 /// Pointer is known to be not null.
 def NonNull : EnumAttr<"nonnull">;
 
 /// The function does not recurse.
 def NoRecurse : EnumAttr<"norecurse">;
 
 /// Disable redzone.
 def NoRedZone : EnumAttr<"noredzone">;

llvm/include/llvm/IR/InstrTypes.h

public:
   }
 
   /// Determine if the invoke cannot be duplicated.
   bool cannotDuplicate() const { return hasFnAttr(Attribute::NoDuplicate); }
   void setCannotDuplicate() {
     addAttribute(AttributeList::FunctionIndex, Attribute::NoDuplicate);
   }
 
+  /// Determine if the invoke cannot be tail merged.
+  bool cannotMerge() const { return hasFnAttr(Attribute::NoMerge); }
+
   /// Determine if the invoke is convergent
   bool isConvergent() const { return hasFnAttr(Attribute::Convergent); }
   void setConvergent() {
     addAttribute(AttributeList::FunctionIndex, Attribute::Convergent);
   }
   void setNotConvergent() {
     removeAttribute(AttributeList::FunctionIndex, Attribute::Convergent);
   }

llvm/lib/IR/Attributes.cpp

std::string Attribute::getAsString(bool InAttrGrp) const {
   if (hasAttribute(Attribute::NoFree))
     return "nofree";
   if (hasAttribute(Attribute::NoImplicitFloat))
     return "noimplicitfloat";
   if (hasAttribute(Attribute::NoInline))
     return "noinline";
   if (hasAttribute(Attribute::NonLazyBind))
     return "nonlazybind";
+  if (hasAttribute(Attribute::NoMerge))
+    return "nomerge";
   if (hasAttribute(Attribute::NonNull))
     return "nonnull";
   if (hasAttribute(Attribute::NoRedZone))
     return "noredzone";
   if (hasAttribute(Attribute::NoReturn))
     return "noreturn";
   if (hasAttribute(Attribute::NoSync))
     return "nosync";

llvm/lib/IR/Verifier.cpp

void Verifier::visitModuleFlagCGProfileEntry(const MDOperand &MDO) {
   auto Count = dyn_cast_or_null<ConstantAsMetadata>(Node->getOperand(2));
   Assert(Count && Count->getType()->isIntegerTy(),
          "expected an integer constant", Node->getOperand(2));
 }
 
 /// Return true if this attribute kind only applies to functions.
 static bool isFuncOnlyAttr(Attribute::AttrKind Kind) {
   switch (Kind) {
+  case Attribute::NoMerge:
   case Attribute::NoReturn:
   case Attribute::NoSync:
   case Attribute::WillReturn:
   case Attribute::NoCfCheck:
   case Attribute::NoUnwind:
   case Attribute::NoInline:
   case Attribute::AlwaysInline:
   case Attribute::OptimizeForSize:

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

do {
     auto *C2 = dyn_cast<CallInst>(I2);
     if (C1 && C2)
       if (C1->isMustTailCall() != C2->isMustTailCall())
         return Changed;
 
     if (!TTI.isProfitableToHoist(I1) || !TTI.isProfitableToHoist(I2))
       return Changed;
 
+    // If any of the two call sites has nomerge attribute, stop hoisting.
+    if (const auto *CB1 = dyn_cast<CallBase>(I1))
+      if (CB1->cannotMerge())
+        return Changed;
+    if (const auto *CB2 = dyn_cast<CallBase>(I2))
+      if (CB2->cannotMerge())
+        return Changed;
+
     if (isa<DbgInfoIntrinsic>(I1) || isa<DbgInfoIntrinsic>(I2)) {
       assert (isa<DbgInfoIntrinsic>(I1) && isa<DbgInfoIntrinsic>(I2));
       // The debug location is an integral part of a debug info intrinsic
       // and can't be separated from it or replaced.  Instead of attempting
       // to merge locations, simply hoist both copies of the intrinsic.
       BIParent->getInstList().splice(BI->getIterator(),
                                      BB1->getInstList(), I1);
       BIParent->getInstList().splice(BI->getIterator(),
for (auto *I : Insts) {
     // These instructions may change or break semantics if moved.
     if (isa<PHINode>(I) || I->isEHPad() || isa<AllocaInst>(I) ||
         I->getType()->isTokenTy())
       return false;
 
     // Conservatively return false if I is an inline-asm instruction. Sinking
     // and merging inline-asm instructions can potentially create arguments
     // that cannot satisfy the inline-asm constraints.
+    // If the instruction has nomerge attribute, return false.
     if (const auto *C = dyn_cast<CallBase>(I))
-      if (C->isInlineAsm())
+      if (C->isInlineAsm() || C->cannotMerge())
         return false;
 
     // Each instruction must have zero or one use.
     if (HasUse && !I->hasOneUse())
       return false;
     if (!HasUse && !I->user_empty())
       return false;
   }