Attach readonly and readnone attributes to inline-asm instructions

[ahatanak]

Summary

Currently, clang/llvm handles inline-asm instructions in a very conservative manner, choosing not to eliminate the instructions or hoisting them out of a loop even when it's safe to do so. This patch makes changes to clang to attach a readonly or readnone attribute to an inline-asm instruction, which enables passes such as LICM and EarlyCSE to move or optimize away the instruction.

The patch is based on the patch Chad proposed in 2012:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120430/057131.html

If this patch is approved, I plan to add another test case to check CSE and LICM can remove an inline-asm or hoist it out of a loop if readonly or readnone is attached.

[ahatanak]

Made changes to the previous patch to stop attaching readonly or readnone in the following cases:

1. The inline-asm has no output operands. In this case, it is implicitly volatile.
2. The inline-asm returns a value by-reference instead of by-value. The previous patch wasn't catching the case where a "=r" constraint resulted in a value being returned by-reference.

Also, fixed the test case to catch those two cases.

[ahatanak]

Ping.

[hfinkel]

Does this mean that if I have two inline asm statements, once which sets a condition-code/model-specific register, and one which reads that register, because neither read or write memory, we'd now be allowed to reorder them? I'm concerned that, because we use memory dependencies to track all dependencies, this will lead to us losing track of other register dependencies that we should otherwise track.

[ahatanak]

Do the inline-asm statements you are thinking of use clobber register list to represent the dependency between the first and second statement? If they are using input and output operands or the volatile keyword, clang will know they can't be reordered, so I was guessing they were using the clobber list?

[hfinkel]

In D10546#202612, @ahatanak wrote:

Do the inline-asm statements you are thinking of use clobber register list to represent the dependency between the first and second statement? If they are using input and output operands or the volatile keyword, clang will know they can't be reordered, so I was guessing they were using the clobber list?

I take back what I said, mostly. GCC's documentation says that, "GCC's optimizers sometimes discard asm statements if they determine there is no need for the output variables. Also, the optimizers may move code out of loops if they believe that the code will always return the same result (i.e. none of its input values change between calls). Using the volatile qualifier disables these optimizations. asm statements that have no output operands, including asm goto statements, are implicitly volatile."

So we're okay here, such inline asm statements need to be marked volatile, or have no output registers, so we can check for that.

I don't think, however, you can ever mark the inline asm call as 'readnone', because you have no way of knowing that it does not load anything. Any of its inputs could be an address, or used to construct an address, to something. The programmer might also know that the address is always dereferenceable, and thus won't produce any side effects other than its result (and, thus, not mark the statement as volatile).

We could parse the assembly using the integrated assembler to figure this out, but that's another story.

[ahatanak]

In D10546#202675, @hfinkel wrote:

I don't think, however, you can ever mark the inline asm call as 'readnone', because you have no way of knowing that it does not load anything. Any of its inputs could be an address, or used to construct an address, to something. The programmer might also know that the address is always dereferenceable, and thus won't produce any side effects other than its result (and, thus, not mark the statement as volatile).

The gcc documentation has the following sentence:

The "memory" clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters).

I took it to mean that you have to add "memory" to an inline-asm statement's clobber list in the case it reads from memory using one of the input registers. If you don't, gcc will treat it as a read-none statement.

asm ("movl (%1), %0" : "=r" (res) : "r" (ptr) : "memory");

[hfinkel]

In D10546#202823, @ahatanak wrote:

In D10546#202675, @hfinkel wrote:

I don't think, however, you can ever mark the inline asm call as 'readnone', because you have no way of knowing that it does not load anything. Any of its inputs could be an address, or used to construct an address, to something. The programmer might also know that the address is always dereferenceable, and thus won't produce any side effects other than its result (and, thus, not mark the statement as volatile).

The gcc documentation has the following sentence:

The "memory" clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters).

Okay, sounds good.

I took it to mean that you have to add "memory" to an inline-asm statement's clobber list in the case it reads from memory using one of the input registers. If you don't, gcc will treat it as a read-none statement.

asm ("movl (%1), %0" : "=r" (res) : "r" (ptr) : "memory");

In that case, LGTM.

cfe/trunk/lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitAsmStmt(const AsmStmt &S) {
   std::vector<llvm::Type *> ArgTypes;
   std::vector<llvm::Value*> Args;
 
   // Keep track of inout constraints.
   std::string InOutConstraints;
   std::vector<llvm::Value*> InOutArgs;
   std::vector<llvm::Type*> InOutArgTypes;
 
+  // An inline asm can be marked readonly if it meets the following conditions:
+  //  - it doesn't have any sideeffects
+  //  - it doesn't clobber memory
+  //  - it doesn't return a value by-reference
+  // It can be marked readnone if it doesn't have any input memory constraints
+  // in addition to meeting the conditions listed above.
+  bool ReadOnly = true, ReadNone = true;
+
   for (unsigned i = 0, e = S.getNumOutputs(); i != e; i++) {
     TargetInfo::ConstraintInfo &Info = OutputConstraintInfos[i];
 
     // Simplify the output constraint.
     std::string OutputConstraint(S.getOutputConstraint(i));
     OutputConstraint = SimplifyConstraint(OutputConstraint.c_str() + 1,
                                           getTarget());
 
if (!Info.allowsMemory() && hasScalarEvaluationKind(OutExpr->getType())) {
                               diag::err_asm_invalid_type_in_input)
             << OutExpr->getType() << OutputConstraint;
       }
     } else {
       ArgTypes.push_back(Dest.getAddress()->getType());
       Args.push_back(Dest.getAddress());
       Constraints += "=*";
       Constraints += OutputConstraint;
+      ReadOnly = ReadNone = false;
     }
 
     if (Info.isReadWrite()) {
       InOutConstraints += ',';
 
       const Expr *InputExpr = S.getOutputExpr(i);
       llvm::Value *Arg = EmitAsmInputLValue(Info, Dest, InputExpr->getType(),
                                             InOutConstraints,
if (isa<MSAsmStmt>(&S)) {
     }
   }
 
   for (unsigned i = 0, e = S.getNumInputs(); i != e; i++) {
     const Expr *InputExpr = S.getInputExpr(i);
 
     TargetInfo::ConstraintInfo &Info = InputConstraintInfos[i];
 
+    if (Info.allowsMemory())
+      ReadNone = false;
+
     if (!Constraints.empty())
       Constraints += ',';
 
     // Simplify the input constraint.
     std::string InputConstraint(S.getInputConstraint(i));
     InputConstraint = SimplifyConstraint(InputConstraint.c_str(), getTarget(),
                                          &OutputConstraintInfos);
 
for (unsigned i = 0, e = InOutArgs.size(); i != e; i++) {
     Args.push_back(InOutArgs[i]);
   }
   Constraints += InOutConstraints;
 
   // Clobbers
   for (unsigned i = 0, e = S.getNumClobbers(); i != e; i++) {
     StringRef Clobber = S.getClobber(i);
 
-    if (Clobber != "memory" && Clobber != "cc")
+    if (Clobber == "memory")
+      ReadOnly = ReadNone = false;
+    else if (Clobber != "cc")
       Clobber = getTarget().getNormalizedGCCRegisterName(Clobber);
 
     if (!Constraints.empty())
       Constraints += ',';
 
     Constraints += "~{";
     Constraints += Clobber;
     Constraints += '}';
llvm::InlineAsm::AsmDialect AsmDialect = isa<MSAsmStmt>(&S) ?
     llvm::InlineAsm::AD_Intel : llvm::InlineAsm::AD_ATT;
   llvm::InlineAsm *IA =
     llvm::InlineAsm::get(FTy, AsmString, Constraints, HasSideEffect,
                          /* IsAlignStack */ false, AsmDialect);
   llvm::CallInst *Result = Builder.CreateCall(IA, Args);
   Result->addAttribute(llvm::AttributeSet::FunctionIndex,
                        llvm::Attribute::NoUnwind);
 
+  // Attach readnone and readonly attributes.
+  if (!HasSideEffect) {
+    if (ReadNone)
+      Result->addAttribute(llvm::AttributeSet::FunctionIndex,
+                           llvm::Attribute::ReadNone);
+    else if (ReadOnly)
+      Result->addAttribute(llvm::AttributeSet::FunctionIndex,
+                           llvm::Attribute::ReadOnly);
+  }
+
   // Slap the source location of the inline asm into a !srcloc metadata on the
   // call.
   if (const GCCAsmStmt *gccAsmStmt = dyn_cast<GCCAsmStmt>(&S)) {
     Result->setMetadata("srcloc", getAsmSrcLocInfo(gccAsmStmt->getAsmString(),
                                                    *this));
   } else {
     // At least put the line number on MS inline asm blobs.
     auto Loc = llvm::ConstantInt::get(Int32Ty, S.getAsmLoc().getRawEncoding());

cfe/trunk/test/CodeGen/asm-attrs.c

+// RUN: %clang_cc1 -triple armv7-apple-darwin -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: call i32 asm "foo0", {{.*}} [[READNONE:#[0-9]+]]
+// CHECK: call i32 asm "foo1", {{.*}} [[READNONE]]
+// CHECK: call i32 asm "foo2", {{.*}} [[NOATTRS:#[0-9]+]]
+// CHECK: call i32 asm sideeffect "foo3", {{.*}} [[NOATTRS]]
+// CHECK: call i32 asm "foo4", {{.*}} [[READONLY:#[0-9]+]]
+// CHECK: call i32 asm "foo5", {{.*}} [[READONLY]]
+// CHECK: call i32 asm "foo6", {{.*}} [[NOATTRS]]
+// CHECK: call void asm sideeffect "foo7", {{.*}} [[NOATTRS]]
+// CHECK: call void asm "foo8", {{.*}} [[NOATTRS]]
+
+// CHECK: attributes [[READNONE]] = { nounwind readnone }
+// CHECK: attributes [[NOATTRS]] = { nounwind }
+// CHECK: attributes [[READONLY]] = { nounwind readonly }
+
+int g0, g1;
+
+struct S {
+  int i;
+} g2;
+
+void test_attrs(int a) {
+  __asm__ ("foo0" : "=r"(g1) : "r"(a));
+  __asm__ ("foo1" : "=r"(g1) : "r"(a) : "cc");
+  __asm__ ("foo2" : "=r"(g1) : "r"(a) : "memory");
+  __asm__ volatile("foo3" : "=r"(g1) : "r"(a));
+  __asm__ ("foo4" : "=r"(g1) : "r"(a), "m"(g0));
+  __asm__ ("foo5" : "=r"(g1) : "r"(a), "Q"(g0));
+  __asm__ ("foo6" : "=r"(g1), "=m"(g0) : "r"(a));
+  __asm__ ("foo7" : : "r"(a));
+  __asm__ ("foo8" : "=r"(g2) : "r"(a));
+}