[CFE] Add nomerge function attribute to inline assembly.

[pengfei]

Summary

Sometimes we also want to avoid merging inline assembly. This patch add
the nomerge function attribute to inline assembly.

[zequanwu]

LGTM.

[xbolva00]

clang/lib/Sema/SemaStmtAttr.cpp

186

This is totally wrong, just big hack to smuggle it here.

[rnk]

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

[pengfei]

In D84225#3302142, @rnk wrote:

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

IIRC, the initial requirment is to avoid the CSE like optimizations. We usually use inline asm for sepcial proposes. We have to stop the merge some time.

clang/lib/Sema/SemaStmtAttr.cpp

186

Could you explain more? Is there any unexpect sideeffect by this?

[lebedev.ri]

In D84225#3303771, @pengfei wrote:

In D84225#3302142, @rnk wrote:

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

IIRC, the initial requirment is to avoid the CSE like optimizations. We usually use inline asm for sepcial proposes. We have to stop the merge some time.

Since the big hammer (nomerge) is already there i suppose this is fine,
but given that there is little context in the original description,
the wording makes it seem like it's being used to workaround
something that may or may not be a bug in the first place.

There isn't anything inherently wrong with merging inlineasm in general,
if that does not break the constraints, especially since
there's already a sideeffect keyword possible on the inlineasm.

[pengfei]

In D84225#3303821, @lebedev.ri wrote:

In D84225#3303771, @pengfei wrote:

In D84225#3302142, @rnk wrote:

I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE).

IIRC, the initial requirment is to avoid the CSE like optimizations. We usually use inline asm for sepcial proposes. We have to stop the merge some time.

Since the big hammer (nomerge) is already there i suppose this is fine,
but given that there is little context in the original description,
the wording makes it seem like it's being used to workaround
something that may or may not be a bug in the first place.

There isn't anything inherently wrong with merging inlineasm in general,
if that does not break the constraints, especially since
there's already a sideeffect keyword possible on the inlineasm.

It's not a workaround. We do need to avoid the merging sometime. For example, given we have 2 branches begin with inline asm of endbr. We have to use nomerge to stop them been merged out of the branches. sideeffect doesn't help with that.

[rnk]

In D84225#3304189, @pengfei wrote:

It's not a workaround. We do need to avoid the merging sometime. For example, given we have 2 branches begin with inline asm of endbr. We have to use nomerge to stop them been merged out of the branches. sideeffect doesn't help with that.

That doesn't sound sufficient to ensure that endbr will be the first instruction in that basic block, which I'm guessing is a requirement. PHI nodes might cause register copies / spills to appear before endbr, and instrumentation passes typically insert code at the top of basic blocks. It sounds like we might need a more complete solution for tracking indirect branch target blocks. Maybe indirectbr and basic block addresses already feed into this, but I'm out of my depth here.

Anyway, I don't want to make a value judgment here. I'm in favor of this change. We should allow users to apply nomerge to inline asm, whether it is a workaround or not.

[pengfei]

In D84225#3305140, @rnk wrote:

In D84225#3304189, @pengfei wrote:

It's not a workaround. We do need to avoid the merging sometime. For example, given we have 2 branches begin with inline asm of endbr. We have to use nomerge to stop them been merged out of the branches. sideeffect doesn't help with that.

That doesn't sound sufficient to ensure that endbr will be the first instruction in that basic block, which I'm guessing is a requirement. PHI nodes might cause register copies / spills to appear before endbr, and instrumentation passes typically insert code at the top of basic blocks. It sounds like we might need a more complete solution for tracking indirect branch target blocks. Maybe indirectbr and basic block addresses already feed into this, but I'm out of my depth here.

Anyway, I don't want to make a value judgment here. I'm in favor of this change. We should allow users to apply nomerge to inline asm, whether it is a workaround or not.

Thanks @rnk . Yes, so we emit endbr in a backend pass rather than this way. I just want to demonstrate why we can't merge inline asm sometime. I don't have a better example at a short time :)

[xbolva00]

clang/lib/Sema/SemaStmtAttr.cpp

186

It looks unfortunate to have something like AsmStmt in "CallExprFinder" with CallExpr as reference to clang's CallExpr.

Kinda surprised that your list of reviewers missed ALL known clang developers/code owner, in this case especially @aaron.ballman .

[aaron.ballman]

clang/lib/Sema/SemaStmtAttr.cpp

186

Yeah, I would have expected that something named CallExprFinder would only find call expressions, not use of inline assembly. The class now seems to be misnamed and that may be surprising to users. This is then being built on top of by things like https://reviews.llvm.org/D119061.

I'm not certain what a reasonable name for the class is given that we now want to use it for different purposes.

[pengfei]

clang/lib/Sema/SemaStmtAttr.cpp

186

Thanks @xbolva00 and @aaron.ballman for the input!
I added it to suppress the diagnosis and it's OK since it's the only use of the class at that time. I'm fine with the change on D119061.

clang/lib/CodeGen/CGStmt.cpp

for (unsigned i = 0, e = StrVal.size() - 1; i != e; ++i) {
           llvm::ConstantInt::get(CGF.Int32Ty, LineLoc.getRawEncoding())));
     }
   }
 
   return llvm::MDNode::get(CGF.getLLVMContext(), Locs);
 }
 
 static void UpdateAsmCallInst(llvm::CallBase &Result, bool HasSideEffect,
-                              bool ReadOnly, bool ReadNone, const AsmStmt &S,
+                              bool ReadOnly, bool ReadNone, bool NoMerge,
+                              const AsmStmt &S,
                               const std::vector<llvm::Type *> &ResultRegTypes,
                               CodeGenFunction &CGF,
                               std::vector<llvm::Value *> &RegResults) {
   Result.addAttribute(llvm::AttributeList::FunctionIndex,
                       llvm::Attribute::NoUnwind);
+  if (NoMerge)
+    Result.addAttribute(llvm::AttributeList::FunctionIndex,
+                        llvm::Attribute::NoMerge);
   // Attach readnone and readonly attributes.
   if (!HasSideEffect) {
     if (ReadNone)
       Result.addAttribute(llvm::AttributeList::FunctionIndex,
                           llvm::Attribute::ReadNone);
     else if (ReadOnly)
       Result.addAttribute(llvm::AttributeList::FunctionIndex,
                           llvm::Attribute::ReadOnly);
llvm::InlineAsm *IA =
     llvm::InlineAsm::get(FTy, AsmString, Constraints, HasSideEffect,
                          /* IsAlignStack */ false, AsmDialect);
   std::vector<llvm::Value*> RegResults;
   if (IsGCCAsmGoto) {
     llvm::CallBrInst *Result =
         Builder.CreateCallBr(IA, Fallthrough, Transfer, Args);
     EmitBlock(Fallthrough);
     UpdateAsmCallInst(cast<llvm::CallBase>(*Result), HasSideEffect, ReadOnly,
-                      ReadNone, S, ResultRegTypes, *this, RegResults);
+                      ReadNone, InNoMergeAttributedStmt, S, ResultRegTypes,
+                      *this, RegResults);
   } else {
     llvm::CallInst *Result =
         Builder.CreateCall(IA, Args, getBundlesForFunclet(IA));
     UpdateAsmCallInst(cast<llvm::CallBase>(*Result), HasSideEffect, ReadOnly,
-                      ReadNone, S, ResultRegTypes, *this, RegResults);
+                      ReadNone, InNoMergeAttributedStmt, S, ResultRegTypes,
+                      *this, RegResults);
   }
 
   assert(RegResults.size() == ResultRegTypes.size());
   assert(RegResults.size() == ResultTruncRegTypes.size());
   assert(RegResults.size() == ResultRegDests.size());
   // ResultRegDests can be also populated by addReturnRegisterOutputs() above,
   // in which case its size may grow.
   assert(ResultTypeRequiresCast.size() <= ResultRegDests.size());

clang/lib/Sema/SemaStmtAttr.cpp

 public:
   typedef ConstEvaluatedExprVisitor<CallExprFinder> Inherited;
 
   CallExprFinder(Sema &S, const Stmt *St) : Inherited(S.Context) { Visit(St); }
 
   bool foundCallExpr() { return FoundCallExpr; }
 
   void VisitCallExpr(const CallExpr *E) { FoundCallExpr = true; }
+  void VisitAsmStmt(const AsmStmt *S) { FoundCallExpr = true; }
 
   void Visit(const Stmt *St) {
     if (!St)
       return;
     ConstEvaluatedExprVisitor<CallExprFinder>::Visit(St);
   }
 };
 } // namespace

clang/test/CodeGen/attr-nomerge.cpp

 // RUN: %clang_cc1 -S -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
 
 bool bar();
 void f(bool, bool);
 
 void foo(int i) {
   [[clang::nomerge]] bar();
   [[clang::nomerge]] (i = 4, bar());
   [[clang::nomerge]] (void)(bar());
   [[clang::nomerge]] f(bar(), bar());
   [[clang::nomerge]] [] { bar(); bar(); }(); // nomerge only applies to the anonymous function call
   [[clang::nomerge]] for (bar(); bar(); bar()) {}
+  [[clang::nomerge]] { asm("nop"); }
   bar();
 }
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR:[0-9]+]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
 // CHECK: call void @_Z1fbb({{.*}}) #[[NOMERGEATTR]]
 // CHECK: call void @"_ZZ3fooiENK3$_0clEv"(%class.anon* %ref.tmp) #[[NOMERGEATTR]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
 // CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
+// CHECK: call void asm {{.*}} #[[NOMERGEATTR2:[0-9]+]]
 // CHECK: call zeroext i1 @_Z3barv()
 // CHECK: attributes #[[NOMERGEATTR]] = { nomerge }
+// CHECK: attributes #[[NOMERGEATTR2]] = { nomerge nounwind }