[Inline asm][GCC compatibility] Handle %v-prefixed code in inline assembly

[• d.zobnin.bugzilla]

Summary

Handle an inline assembly feature of GCC: code prefixed with "%v", e. g. "%vpcmpestri" is transformed into "vpcmpestri" instruction if target supports AVX and into "pcmpestri" otherwise.

Given the code:

void f(void* arg)
{
  __asm__ ("%vpcmpestri $0, (%1), %2"
           : "=c"(arg)
           : "r"(arg), "x"(arg));
}

"gcc -c test.c -o test.o" produces

movq   -0x10(%rbp),%xmm0
pcmpestri $0x0,(%rax),%xmm0

While "gcc -c -march=corei7-avx test.c -o test.o" produces

vmovq  %rdx,%xmm0
vpcmpestri $0x0,(%rax),%xmm0

[echristo]

That's fascinating and terrifying all at once.

Can you rename the test to avx-v-modifier-inline-asm.c please?

One inline comment as well.

Thanks!

-eric

lib/AST/Stmt.cpp

639

This should probably check the feature set based on the function rather than the feature set of the base compile yes? What does gcc do if you put it in an attribute((target("..."))) function?

[• d.zobnin.bugzilla]

Eric, thank you for the review!

I've renamed the test, but have also made it a .cpp file for the reasons described below.

You are right -- GCC looks at "target" attribute applied to the function in which inline asm exists. But GCC seems to take into account lambda's attributes as well, if asm statement is inside a lambda. To be correct, GCC seems to collect all target features from command line, function and lambda's attributes, so such code:

void f8(void *x) __attribute__((__target__("no-avx")));
void f8(void *x) {
  auto g1 = [](void *arg) __attribute__((__target__("avx2"))) {
    auto g2 = [&arg]() __attribute__((__target__("no-mmx"))) {
      int a = 10, b;
      __asm__ ("%vmovq (%%rbp), %%xmm0" : "=r"(b) : "r"(a) : "%eax");
    };
    g2();
  };
  g1(x);
}

will produce "vmovq" instruction regardless of command line arguments because of g1's attribute.

I tried several approaches to implement this behavior and came up with this patch, please take a look!

[• d.zobnin.bugzilla]

Friendly ping, please take a look.

Thank you,
Denis Zobnin

[• d.zobnin.bugzilla]

This is just a weekly friendly ping.
Please take a look at the new patch!

Thank you,
Denis Zobnin

[• d.zobnin.bugzilla]

Updated the patch after r266292 commit: [ASTImporter] Implement some expression-related AST node import.

[echristo]

Hi,

Couple of inline comments. The direction is getting there, but there's some overall architecture issues to sort out I think.

Thanks!

-eric

include/clang/AST/Stmt.h

1555

I'd prefer not to keep SupportsAVX cached, or pass it though, but rather look up what we need from the function if we need it. We can also cache the set of valid subtarget features on the Stmt if necessary from the Function.

lib/Sema/SemaStmtAsm.cpp

144–146

Hate to say, but this is the point that we now have 3 of these scattered around - can you take a look at what it will take to merge all of these in a way that we can look up the cached features?

include/clang/AST/Stmt.h

class GCCAsmStmt : public AsmStmt {
   SourceLocation RParenLoc;
   StringLiteral *AsmStr;
 
   // FIXME: If we wanted to, we could allocate all of these in one big array.
   StringLiteral **Constraints;
   StringLiteral **Clobbers;
   IdentifierInfo **Names;
 
+  // Indicates whether this statement exists inside a function with enabled AVX
+  // target feature (by attribute or invocation options).
+  bool SupportsAVX;
+
   friend class ASTStmtReader;
 
 public:
   GCCAsmStmt(const ASTContext &C, SourceLocation asmloc, bool issimple,
-             bool isvolatile, unsigned numoutputs, unsigned numinputs,
-             IdentifierInfo **names, StringLiteral **constraints, Expr **exprs,
-             StringLiteral *asmstr, unsigned numclobbers,
-             StringLiteral **clobbers, SourceLocation rparenloc);
+             bool isvolatile, bool supportsavx, unsigned numoutputs,
+             unsigned numinputs, IdentifierInfo **names,
+             StringLiteral **constraints, Expr **exprs, StringLiteral *asmstr,
+             unsigned numclobbers, StringLiteral **clobbers,
+             SourceLocation rparenloc);
 
   /// \brief Build an empty inline-assembly statement.
-  explicit GCCAsmStmt(EmptyShell Empty) : AsmStmt(GCCAsmStmtClass, Empty),
-    Constraints(nullptr), Clobbers(nullptr), Names(nullptr) { }
+  explicit GCCAsmStmt(EmptyShell Empty)
+      : AsmStmt(GCCAsmStmtClass, Empty), Constraints(nullptr),
+        Clobbers(nullptr), Names(nullptr), SupportsAVX(false) {}
 
   SourceLocation getRParenLoc() const { return RParenLoc; }
   void setRParenLoc(SourceLocation L) { RParenLoc = L; }
 
+  bool getSupportsAVX() const { return SupportsAVX; }
+  void setSupportsAVX(bool supportsAVX) { SupportsAVX = supportsAVX; }
+
   //===--- Asm String Analysis ---===//
 
   const StringLiteral *getAsmString() const { return AsmStr; }
   StringLiteral *getAsmString() { return AsmStr; }
   void setAsmString(StringLiteral *E) { AsmStr = E; }
 
   /// AsmStringPiece - this is part of a decomposed asm string specification
   /// (for use with the AnalyzeAsmString function below).  An asm string is

lib/AST/ASTImporter.cpp

Stmt *ASTNodeImporter::VisitGCCAsmStmt(GCCAsmStmt *S) {
   if (!AsmStr)
     return nullptr;
 
   return new (Importer.getToContext()) GCCAsmStmt(
         Importer.getToContext(),
         Importer.Import(S->getAsmLoc()),
         S->isSimple(),
         S->isVolatile(),
+        S->getSupportsAVX(),
         S->getNumOutputs(),
         S->getNumInputs(),
         Names.data(),
         Constraints.data(),
         Exprs.data(),
         AsmStr,
         S->getNumClobbers(),
         Clobbers.data(),

lib/AST/Stmt.cpp

if (EscapedChar == '[') {
           getAsmString()->getLocationOfByte(Percent - StrStart, SM, LO, TI);
       SourceLocation EndLoc =
           getAsmString()->getLocationOfByte(NameEnd + 1 - StrStart, SM, LO, TI);
 
       Pieces.emplace_back(N, std::move(Str), BeginLoc, EndLoc);
 
       CurPtr = NameEnd+1;
       continue;
+    } else if (*Begin == 'v') {
+      // GCC accepts code staring with "%v", e. g. "%vpcmpestri" and transforms
+      // it into "vpcmpestri" instruction if target processor supports AVX and
+      // into "pcmpestri" otherwise.
+      if (SupportsAVX)
+        CurStringPiece = "v" + CurStringPiece;
+      CurStringPiece += EscapedChar;
+      continue;
     }
 
     DiagOffs = CurPtr-StrStart-1;
     return diag::err_asm_invalid_escape;
   }
 }
 
 /// Assemble final IR asm string (GCC-style).
void MSAsmStmt::setInputExpr(unsigned i, Expr *E) {
   Exprs[i + NumOutputs] = E;
 }
 
 //===----------------------------------------------------------------------===//
 // Constructors
 //===----------------------------------------------------------------------===//
 
 GCCAsmStmt::GCCAsmStmt(const ASTContext &C, SourceLocation asmloc,
-                       bool issimple, bool isvolatile, unsigned numoutputs,
-                       unsigned numinputs, IdentifierInfo **names,
-                       StringLiteral **constraints, Expr **exprs,
-                       StringLiteral *asmstr, unsigned numclobbers,
-                       StringLiteral **clobbers, SourceLocation rparenloc)
+                       bool issimple, bool isvolatile, bool supportsavx,
+                       unsigned numoutputs, unsigned numinputs,
+                       IdentifierInfo **names, StringLiteral **constraints,
+                       Expr **exprs, StringLiteral *asmstr,
+                       unsigned numclobbers, StringLiteral **clobbers,
+                       SourceLocation rparenloc)
     : AsmStmt(GCCAsmStmtClass, asmloc, issimple, isvolatile, numoutputs,
-            numinputs, numclobbers), RParenLoc(rparenloc), AsmStr(asmstr) {
+              numinputs, numclobbers),
+      RParenLoc(rparenloc), AsmStr(asmstr), SupportsAVX(supportsavx) {
 
   unsigned NumExprs = NumOutputs + NumInputs;
 
   Names = new (C) IdentifierInfo*[NumExprs];
   std::copy(names, names + NumExprs, Names);
 
   Exprs = new (C) Stmt*[NumExprs];
   std::copy(exprs, exprs + NumExprs, Exprs);

lib/Sema/SemaStmtAsm.cpp

S.Diag(E->getLocStart(), diag::err_asm_non_addr_value_in_memory_constraint)
         << EType << is_input_expr << Info.getConstraintStr()
         << E->getSourceRange();
     return true;
   }
 
   return false;
 }
 
+// Collect all target features specified for the current context by the "target"
+// attribute. Start from current DeclContext and iterate its parents until we
+// find a function context.
+static void CollectTargetFeatures(Sema &S,
+                                  std::vector<std::string> &TargetFeatures,
+                                  StringRef &TargetCPU) {
+  DeclContext *DC = S.CurContext;
+
+  auto Collect = [&TargetFeatures, &TargetCPU](const FunctionDecl *FD) {
+    if (!FD)
+      return;
+    if (const auto *TA = FD->getAttr<TargetAttr>()) {
+      TargetAttr::ParsedTargetAttr ParsedAttr = TA->parse();
+      TargetFeatures.insert(TargetFeatures.begin(), ParsedAttr.first.begin(),
+                            ParsedAttr.first.end());
+      if (TargetCPU == "")
+        TargetCPU = ParsedAttr.second;
+    }
+  };
+
+  // Climb up the tree of nested DeclContext-s, collecting the target features.
+  while (true) {
+    if (isa<BlockDecl>(DC) || isa<EnumDecl>(DC) || isa<CapturedDecl>(DC)) {
+      DC = DC->getParent();
+    } else if (const auto *MD = dyn_cast<CXXMethodDecl>(DC)) {
+      // Check if it's a lambda and collect its target features.
+      if (MD->getOverloadedOperator() == OO_Call &&
+          cast<CXXRecordDecl>(DC->getParent())->isLambda()) {
+        Collect(MD);
+      }
+      DC = DC->getParent()->getParent();
+    } else {
+      // It's the nearest function, collect its features and break.
+      Collect(dyn_cast<FunctionDecl>(DC));
+      break;
+    }
+  }
+}
+
+// Check whether a particular target feature is enabled in current context,
+// either by command-line options or by __attribute__((__target__("..."))),
+// applied to any surrounding lambda or function.
+static bool TargetFeatureEnabled(Sema &S, StringRef Feature) {
+  const TargetInfo &TI = S.Context.getTargetInfo();
+  llvm::StringMap<bool> FeatureMap;
+  StringRef TargetCPU;
+  std::vector<std::string> TargetFeatures;
+
+  // Collect target features and CPU from surrounding declarations.
+  CollectTargetFeatures(S, TargetFeatures, TargetCPU);
+
+  // Don't forget to add command-line features and CPU (lowest priority).
+  TargetFeatures.insert(TargetFeatures.begin(),
+                        TI.getTargetOpts().FeaturesAsWritten.begin(),
+                        TI.getTargetOpts().FeaturesAsWritten.end());
+  if (TargetCPU == "")
+    TargetCPU = TI.getTargetOpts().CPU;
+
+  // Process the feature vector.
+  TI.initFeatureMap(FeatureMap, S.getDiagnostics(), TargetCPU, TargetFeatures);
+
+  // See if the requested feature is present and enabled.
+  for (const auto & F : FeatureMap)
+    if (F.getValue() && F.getKey().equals(Feature))
+      return true;
+
+  return false;
+}
+
 StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                                  bool IsVolatile, unsigned NumOutputs,
                                  unsigned NumInputs, IdentifierInfo **Names,
                                  MultiExprArg constraints, MultiExprArg Exprs,
                                  Expr *asmString, MultiExprArg clobbers,
                                  SourceLocation RParenLoc) {
   unsigned NumClobbers = clobbers.size();
   StringLiteral **Constraints =
     reinterpret_cast<StringLiteral**>(constraints.data());
   StringLiteral *AsmString = cast<StringLiteral>(asmString);
   StringLiteral **Clobbers = reinterpret_cast<StringLiteral**>(clobbers.data());
 
+  // Determine if current asm statement exists inside a function with enabled
+  // "AVX" target feature. Use this information to resolve GCC-style statements
+  // with "%v" prefix, transforming "%vfoo" into "vfoo" with enabled AVX and
+  // into "foo" otherwise.
+  bool SupportsAVX = TargetFeatureEnabled(*this, "avx");
+
   SmallVector<TargetInfo::ConstraintInfo, 4> OutputConstraintInfos;
 
   // The parser verifies that there is a string literal here.
   assert(AsmString->isAscii());
 
   // If we're compiling CUDA file and function attributes indicate that it's not
   // for this compilation side, skip all the checks.
   if (!DeclAttrsMatchCUDAMode(getLangOpts(), getCurFunctionDecl())) {
-    GCCAsmStmt *NS = new (Context) GCCAsmStmt(
-        Context, AsmLoc, IsSimple, IsVolatile, NumOutputs, NumInputs, Names,
-        Constraints, Exprs.data(), AsmString, NumClobbers, Clobbers, RParenLoc);
+    GCCAsmStmt *NS = new (Context)
+        GCCAsmStmt(Context, AsmLoc, IsSimple, IsVolatile, SupportsAVX,
+                   NumOutputs, NumInputs, Names, Constraints, Exprs.data(),
+                   AsmString, NumClobbers, Clobbers, RParenLoc);
     return NS;
   }
 
   for (unsigned i = 0; i != NumOutputs; i++) {
     StringLiteral *Literal = Constraints[i];
     assert(Literal->isAscii());
 
     StringRef OutputName;
for (unsigned i = 0; i != NumClobbers; i++) {
 
     StringRef Clobber = Literal->getString();
 
     if (!Context.getTargetInfo().isValidClobber(Clobber))
       return StmtError(Diag(Literal->getLocStart(),
                   diag::err_asm_unknown_register_name) << Clobber);
   }
 
-  GCCAsmStmt *NS =
-    new (Context) GCCAsmStmt(Context, AsmLoc, IsSimple, IsVolatile, NumOutputs,
-                             NumInputs, Names, Constraints, Exprs.data(),
-                             AsmString, NumClobbers, Clobbers, RParenLoc);
+  GCCAsmStmt *NS = new (Context)
+      GCCAsmStmt(Context, AsmLoc, IsSimple, IsVolatile, SupportsAVX, NumOutputs,
+                 NumInputs, Names, Constraints, Exprs.data(), AsmString,
+                 NumClobbers, Clobbers, RParenLoc);
   // Validate the asm string, ensuring it makes sense given the operands we
   // have.
   SmallVector<GCCAsmStmt::AsmStringPiece, 8> Pieces;
   unsigned DiagOffs;
   if (unsigned DiagID = NS->AnalyzeAsmString(Pieces, Context, DiagOffs)) {
     Diag(getLocationOfStringLiteralByte(AsmString, DiagOffs), DiagID)
            << AsmString->getSourceRange();
     return StmtError();

test/CodeGen/avx-v-modifier-inline-asm.cpp

+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux -target-feature +avx -emit-llvm -S %s -o - | FileCheck %s -check-prefix=CHECK-AVX
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-linux -emit-llvm -S %s -o - | FileCheck %s
+
+void f1(void *arg) {
+  __asm__ ("%vpcmpestri $0, (%1), %2" : "=c"(arg) : "r"(arg), "x"(arg));
+  // CHECK-LABEL: f1
+  // CHECK: pcmpestri
+  // CHECK-AVX: vpcmpestri
+}
+
+// Regardless of compiler's invocation options, we should have "vpcmpestri".
+void f2(void *arg) __attribute__((__target__("avx")));
+void f2(void *arg) {
+  __asm__ ("%vpcmpestri $0, (%1), %2" : "=c"(arg) : "r"(arg), "x"(arg));
+  // CHECK-LABEL: f2
+  // CHECK: vpcmpestri
+  // CHECK-AVX: vpcmpestri
+}
+
+// avx2 enables avx, so always expect "vpcmpestri".
+void f3(void *arg) __attribute__((__target__("avx2")));
+void f3(void *arg) {
+  __asm__ ("%vpcmpestri $0, (%1), %2" : "=c"(arg) : "r"(arg), "x"(arg));
+  // CHECK-LABEL: f3
+  // CHECK: vpcmpestri
+  // CHECK-AVX: vpcmpestri
+}
+
+// Here we explicitly disable avx, so always expect "pcmpestri".
+void f4(void *arg) __attribute__((__target__("no-avx")));
+void f4(void *arg) {
+  __asm__ ("%vpcmpestri $0, (%1), %2" : "=c"(arg) : "r"(arg), "x"(arg));
+  // CHECK-LABEL: f4
+  // CHECK: pcmpestri
+  // CHECK-AVX: pcmpestri
+}
+
+// Check that enabling features other than avx don't affect the expected logic.
+void f5(void *arg) __attribute__((__target__("mmx"))) __attribute__((__target__("sse4.1")));
+void f5(void *arg) {
+  __asm__ ("%vpcmpestri $0, (%1), %2" : "=c"(arg) : "r"(arg), "x"(arg));
+  // CHECK-LABEL: f5
+  // CHECK: pcmpestri
+  // CHECK-AVX: vpcmpestri
+}
+
+void f6() {
+  int a = 10, b;
+  __asm__ ("%vmovq (%%rbp), %%xmm0" : "=r"(b) : "r"(a) : "%eax");
+  // CHECK-LABEL: f6
+  // CHECK: movq
+  // CHECK-AVX: vmovq
+}
+
+// Lambda's attributes also counts.
+void f7(void *x) {
+  auto f = [](void *arg) __attribute__((__target__("avx"))) {
+    __asm__("%vpcmpestri $0, (%1), %2" : "=c"(arg) : "r"(arg), "x"(arg));
+  };
+
+  // CHECK-LABEL: f7
+  // CHECK: vpcmpestri
+  // CHECK-AVX: vpcmpestri
+
+  f(x);
+}
+
+// Check influence of attributes of nested scopes. Here command line enables
+// avx, f8 disables, then g1 enables again, so always expect "vmovq".
+void f8(void *x) __attribute__((__target__("no-avx")));
+void f8(void *x) {
+  auto g1 = [](void *arg) __attribute__((__target__("avx2"))) {
+    auto g2 = [&arg]() __attribute__((__target__("no-mmx"))) {
+      int a = 10, b;
+      __asm__ ("%vmovq (%%rbp), %%xmm0" : "=r"(b) : "r"(a) : "%eax");
+
+      // CHECK-LABEL: f8
+      // CHECK: vmovq
+      // CHECK-AVX: vmovq
+    };
+
+    g2();
+  };
+
+  g1(x);
+}