[Clang] Make nomerge attribute a function attribute as well as a statement attribute.

[zequanwu]

Summary

Make nomerge attribute a function attribute as well as a statement attribute to extend the functionality of nomerge attribute.

For functions already have nomerge attribute, don't add nomerge attribute at the call-site of this function.

[zequanwu]

start a new diff to make nomerge attribute a function attribute as well as a statement attribute.

[rnk]

Nice, that wasn't too difficult. I had some suggestions for improving the test case, and I'd like to hear from Aaron.

clang/test/CodeGen/attr-nomerge.cpp

8 ↗

(On Diff #310760)

Hm, virtual functions, there's something worth thinking about. In this case, the attribute must appear on the call site if we want it to work as the user expects. I don't see the indirect call site for the virtual call in the IR below. I think it's worth testing for that. Consider taking A* and B* parameters to defeat any frontend devirtualization optimizations.

17 ↗

(On Diff #310760)

I would leave this declaration as it was, so that all the statement attribute test cases you wrote below remain as they were, and the attribute continues to appear at the call site.

The f declaration below gives you coverage of the free function declaration case.

[zequanwu]

Add test for virutal function.

clang/test/CodeGen/attr-nomerge.cpp

8 ↗

(On Diff #310760)

If devirtualization failed, then only statement attribute would work.

[zequanwu]

clang/test/CodeGen/attr-nomerge.cpp

17 ↗

(On Diff #310760)

I added [[clang::nomerge]] to bar in order to test that the generated IR should attach nomerge to call-sites which callee function already has this attribute.

[rnk]

clang/test/CodeGen/attr-nomerge.cpp

8 ↗

(On Diff #310760)

Fair enough, that is similar to how not_tail_called works. Can you also update AttrDocs.td to mention the limitation?

17 ↗

(On Diff #310760)

Well, from the tests below, it seems like the attribute is *not* present on the call sites. Is that what you intended?

[zequanwu]

Add docs.

clang/test/CodeGen/attr-nomerge.cpp

17 ↗

(On Diff #310760)

oops, I meant "shouldn't attach", because it's not necessary to have it.

[aaron.ballman]

In general, I think this is reasonable -- there's a bit more work to be done before it's ready to land, but this is heading in the right direction.

clang/include/clang/Basic/Attr.td

559

I think this should be an InheritableAttr, like DeclOrTypeAttr. With type/decl attributes, those are almost always for things like calling conventions or other cases where inheritance of the attribute is really likely to be the correct default. It's less clear to me that the same is true for stmt/decl attributes aside from the observation that most decl attributes are inheritable. We may need to come up with a better approach for inheritance here at some point when we find a stmt/decl attribute that should not be inheritable.

clang/lib/Sema/SemaDeclAttr.cpp

2185

No need for this function, you can set let SimpleHandler = 1; in Attr.td -- the common attribute handler code should handle checking the subject list for you automatically.

7747

Then this can become handleSimpleAttribute<NoMergeAttr>(S, D, AL);

clang/test/CodeGen/attr-nomerge.cpp

73 ↗

(On Diff #311062)

Can you also add a test case to demonstrate that inheriting the attribute on a later redeclaration works? It can either be a codegen test or an AST dump test.

clang/test/Sema/attr-nomerge.cpp

11 ↗

(On Diff #311062)

This diagnostic is no longer correct -- nomerge now applies to more things. You may need to teach the clang attribute emitter about stmt/decl attribute types so that it can generate a better diagnostic in diagnoseAppertainsTo().

[zequanwu]

address comments.

clang/test/CodeGen/attr-nomerge.cpp

73 ↗

(On Diff #311062)

Do you mean something like this?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

1318

Related to my comments in ClangAttrEmitter.cpp, I think you should make these changes instead.

clang/test/CodeGen/attr-nomerge.cpp

73 ↗

(On Diff #311062)

Close. I was thinking something like:

int g(int i); // No attr

void something() {
  // call g() here
}

[[clang::nomerge]] int g(int i);

void something_else() {
  // call g() here
}

int g(int i) { return i; }

void something_else_again() {
  // call g() here
}

So this tests that the attribute is inherited on redeclarations properly and it also tests the merging behavior when the declaration may not have yet had the attribute attached.

clang/utils/TableGen/ClangAttrEmitter.cpp

3438–3439 ↗

(On Diff #312078)

I think this will do the wrong thing when the subject list has more than one entry (I think this will add statements to the list once for each subject). As an experiment, can you add ObjCMethod to the list of subjects for NoMerge in Attr.td? Do the resulting diagnostics list "statements" once or twice?

Thinking a bit deeper on this -- I think my original advice may have been too ambitious (we don't yet have support for automatically grabbing the names of statements for diagnostics like we do for declarations). Trying to add that as part of this patch would be a big ask, so I think a better approach is to use a custom diagnostic in Attr.td. I'll add a comment there to this effect.

[zequanwu]

address comments.

clang/utils/TableGen/ClangAttrEmitter.cpp

3438–3439 ↗

(On Diff #312078)

You're right. If I just add ObjCMethod to the list of subjects for NoMerge, the "statements" shows twice in the diag.

[aaron.ballman]

LGTM! Thank you for this!

clang/include/clang/Basic/Attr.td

 
 /// A type attribute is not processed on a declaration or a statement.
 class TypeAttr : Attr;
 
 /// A stmt attribute is not processed on a declaration or a type.
 class StmtAttr : Attr;
 
 /// An inheritable attribute is inherited by later redeclarations.
 class InheritableAttr : Attr {
   // Set to true if this attribute can be duplicated on a subject when inheriting
   // attributes from prior declarations.
   bit InheritEvenIfAlreadyPresent = 0;
 }
 
 /// Some attributes, like calling conventions, can appear in either the
 /// declaration or the type position. These attributes are morally type
 /// attributes, but have historically been written on declarations.

 def Unlikely : StmtAttr {
   let Spellings = [CXX11<"", "unlikely", 201803>, C2x<"clang", "unlikely">];
   let Documentation = [LikelihoodDocs];
 }
 
 def NoMerge : StmtAttr {
   let Spellings = [Clang<"nomerge">];
   let Documentation = [NoMergeDocs];
 }
 
 def FastCall : DeclOrTypeAttr {
   let Spellings = [GCC<"fastcall">, Keyword<"__fastcall">,
                    Keyword<"_fastcall">];
 //  let Subjects = [Function, ObjCMethod];
   let Documentation = [FastCallDocs];
 }
 

 
 def NoInline : InheritableAttr {
   let Spellings = [GCC<"noinline">, Declspec<"noinline">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
+def DisableMerge : InheritableAttr {
+  let Spellings = [Clang<"disable_merge">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [DisableMergeDocs];
+  let SimpleHandler = 1;
+}
+
 def NoMips16 : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"nomips16">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
 def NoMicroMips : InheritableAttr, TargetSpecificAttr<TargetMips32> {

clang/include/clang/Basic/AttrDocs.td

public:
       int foo1() override {
         return callee(); // This call is tail-call optimized.
       }
     };
 
   }];
 }
 
+def DisableMergeDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Use this attribute to prevent multiple calls to the specified function from 
+merging during optimization.
+  }];
+}
+
 def AnyX86NoCallerSavedRegistersDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Use this attribute to indicate that the specified function has no
 caller-saved registers. That is, all registers are callee-saved except for
 registers used for passing parameters to the function or returning parameters
 from the function.
 The compiler saves and restores any modified registers that were not used for

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<DisableMergeAttr>())
+      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/lib/Sema/SemaDeclAttr.cpp

static void handleDestructorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   uint32_t priority = DestructorAttr::DefaultPriority;
   if (AL.getNumArgs() &&
       !checkUInt32Argument(S, AL, AL.getArgAsExpr(0), priority))
     return;
 
   D->addAttr(::new (S.Context) DestructorAttr(S.Context, AL, priority));
 }
 
 template <typename AttrTy>
 static void handleAttrWithMessage(Sema &S, Decl *D, const ParsedAttr &AL) {
   // Handle the case where the attribute has a text message.
   StringRef Str;
   if (AL.getNumArgs() == 1 && !S.checkStringLiteralArgumentAttr(AL, 0, Str))
     return;
 
   D->addAttr(::new (S.Context) AttrTy(S.Context, AL, Str));
 }
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   case ParsedAttr::AT_NotTailCalled:
     handleSimpleAttributeWithExclusions<NotTailCalledAttr, AlwaysInlineAttr>(
         S, D, AL);
     break;
   case ParsedAttr::AT_DisableTailCalls:
     handleSimpleAttributeWithExclusions<DisableTailCallsAttr, NakedAttr>(S, D,
                                                                          AL);
     break;
+  case ParsedAttr::AT_DisableMerge:
+    handleSimpleAttribute<DisableMergeAttr>(S, D, AL);
+    break;
   case ParsedAttr::AT_Visibility:
     handleVisibilityAttr(S, D, AL, false);
     break;
   case ParsedAttr::AT_TypeVisibility:
     handleVisibilityAttr(S, D, AL, true);
     break;
   case ParsedAttr::AT_WarnUnusedResult:
     handleWarnUnusedResult(S, D, AL);

clang/test/CodeGen/attr-disable-merge.cpp

+// RUN: %clang_cc1 -S -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+
+[[clang::disable_merge]] void bar();
+
+void foo(int i) {
+  bar();
+}
+
+// CHECK: call void  @_Z3barv() #[[NOMERGEATTR2:[0-9]+]]
+// CHECK: declare void @_Z3barv() #[[NOMERGEATTR:[0-9]+]]
+// CHECK: attributes #[[NOMERGEATTR]] = { nomerge {{.*}}}
+// CHECK: attributes #[[NOMERGEATTR2]] = { nomerge }

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

 // CHECK-NEXT: Constructor (SubjectMatchRule_function)
 // CHECK-NEXT: Consumable (SubjectMatchRule_record)
 // CHECK-NEXT: ConsumableAutoCast (SubjectMatchRule_record)
 // CHECK-NEXT: ConsumableSetOnRead (SubjectMatchRule_record)
 // CHECK-NEXT: Convergent (SubjectMatchRule_function)
 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: Destructor (SubjectMatchRule_function)
+// CHECK-NEXT: DisableMerge (SubjectMatchRule_function)
 // CHECK-NEXT: DisableTailCalls (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: EnableIf (SubjectMatchRule_function)
 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
 // CHECK-NEXT: ExcludeFromExplicitInstantiation (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
 // CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
 // CHECK-NEXT: Flatten (SubjectMatchRule_function)
 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)

clang/test/Sema/attr-disable-merge.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int foo __attribute__((disable_merge)); // expected-warning {{'disable_merge' attribute only applies to functions}}
+
+int bar(int a) __attribute__((disable_merge("abc"))); // expected-error {{'disable_merge' attribute takes no arguments}}

clang/test/SemaCXX/attr-disable-merge.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+class A {
+public:
+  [[clang::disable_merge]] virtual int foo1() { return 1; }
+  [[clang::disable_merge]] int foo2() { return 2; }
+};
 No newline at end of file

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 merging for call sites
+/// Disable merging for specified function or call sites.
 def NoMerge : EnumAttr<"nomerge">;
 
 /// Pointer is known to be not null.
 def NonNull : EnumAttr<"nonnull">;
 
 /// The function does not recurse.
 def NoRecurse : EnumAttr<"norecurse">;