[X86, inlineasm] Improve analysis of x,Y0,Yi,Ym,Yt,L,e,Z,s asm constraints

[alexfrol]

Summary

Improve Sema checking of 9 existing inline asm constraints (‘x’, ‘Y*’, ‘L’, ‘e’, ‘Z’, ‘s’).

[alexfrol]

Ping!

[echristo]

Sorry for the delay, I'll get to this soon.

Thanks!

-eric

[alexfrol]

Gentle ping

[alexfrol]

Ping!!

[compnerd]

LGTM with the small change to rename isValidAsmValue at least.

include/clang/Basic/TargetInfo.h

597–600

This would be better named as isValidAsmImmediate.

lib/Basic/Targets.cpp

3460

Might be nice to have the interface take an ArrayRef instead.

test/Sema/inline-asm-validate-x86.c

56

Invalid perhaps (instead of NotValid).

[echristo]

This will need documentation updates yes? Also backend support?

-eric

[echristo]

I don't see a test for the 'k' support or a few other things either. Can you split this out a bit into the various parts for checking the existing constraints, and adding individual ones?

-eric

[alexfrol]

In D10536#203421, @compnerd wrote:

LGTM with the small change to rename isValidAsmValue at least.

All issues are fixed. I'll update the patch soon.

In D10536#203820, @echristo wrote:

This will need documentation updates yes? Also backend support?

-eric

What documentation updates do you mean?
Backend support is necessary for ‘v’, ‘k’, and ‘Yk’ constraints. They are AVX-512 specific and as far as I know work for AVX512 backend support is still ongoing.

In D10536#203821, @echristo wrote:

I don't see a test for the 'k' support or a few other things either. Can you split this out a bit into the various parts for checking the existing constraints, and adding individual ones?

-eric

Sure, will do.

[alexfrol]

Removed code for adding individual constraints (separate patch will be submitted later), fixed several reported issues

[echristo]

LGTM, thanks so much for the work here!

-eric

[alexfrol]

Thank you, Eric and Saleem!
Glad to be useful :)

include/clang/Basic/TargetInfo.h

 
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetCXXABI.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/VersionTuple.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/DataTypes.h"
 #include <cassert>
 #include <string>
 #include <vector>
enum {
       CI_EarlyClobber = 0x20,      // "&" output constraint (early clobber).
     };
     unsigned Flags;
     int TiedOperand;
     struct {
       int Min;
       int Max;
     } ImmRange;
+    llvm::SmallSet<int, 4> ImmSet;
 
     std::string ConstraintStr;  // constraint: "=rm"
     std::string Name;           // Operand name: [foo] with no []'s.
   public:
     ConstraintInfo(StringRef ConstraintStr, StringRef Name)
         : Flags(0), TiedOperand(-1), ConstraintStr(ConstraintStr.str()),
           Name(Name.str()) {
       ImmRange.Min = ImmRange.Max = 0;
public:
     unsigned getTiedOperand() const {
       assert(hasTiedOperand() && "Has no tied operand!");
       return (unsigned)TiedOperand;
     }
 
     bool requiresImmediateConstant() const {
       return (Flags & CI_ImmediateConstant) != 0;
     }
-    int getImmConstantMin() const { return ImmRange.Min; }
-    int getImmConstantMax() const { return ImmRange.Max; }
+    bool isValidAsmImmediate(const llvm::APInt &Value) const {
+      return (Value.sge(ImmRange.Min) && Value.sle(ImmRange.Max)) ||
+             ImmSet.count(Value.getZExtValue()) != 0;
+    }
 
     void setIsReadWrite() { Flags |= CI_ReadWrite; }
     void setEarlyClobber() { Flags |= CI_EarlyClobber; }
     void setAllowsMemory() { Flags |= CI_AllowsMemory; }
     void setAllowsRegister() { Flags |= CI_AllowsRegister; }
     void setHasMatchingInput() { Flags |= CI_HasMatchingInput; }
     void setRequiresImmediate(int Min, int Max) {
       Flags |= CI_ImmediateConstant;
       ImmRange.Min = Min;
       ImmRange.Max = Max;
     }
+    void setRequiresImmediate(llvm::ArrayRef<int> Exacts) {
+      Flags |= CI_ImmediateConstant;
+      for (int Exact : Exacts)
+        ImmSet.insert(Exact);
+    }
+    void setRequiresImmediate(int Exact) {
+      Flags |= CI_ImmediateConstant;
+      ImmSet.insert(Exact);
+    }
+    void setRequiresImmediate() {
+      Flags |= CI_ImmediateConstant;
+      ImmRange.Min = INT_MIN;
+      ImmRange.Max = INT_MAX;
+    }
 
     /// \brief Indicate that this is an input operand that is tied to
     /// the specified output operand. 
     ///
     /// Copy over the various constraint information from the output.
     void setTiedOperand(unsigned N, ConstraintInfo &Output) {
       Output.setHasMatchingInput();
       Flags = Output.Flags;

lib/Basic/Targets.cpp

return llvm::StringSwitch<bool>(FeatureStr)
       .Default(false);
 }
 
 bool
 X86TargetInfo::validateAsmConstraint(const char *&Name,
                                      TargetInfo::ConstraintInfo &Info) const {
   switch (*Name) {
   default: return false;
+  // Constant constraints.
+  case 'e': // 32-bit signed integer constant for use with sign-extending x86_64
+            // instructions.
+  case 'Z': // 32-bit unsigned integer constant for use with zero-extending
+            // x86_64 instructions.
+  case 's':
+    Info.setRequiresImmediate();
+    return true;
   case 'I':
     Info.setRequiresImmediate(0, 31);
     return true;
   case 'J':
     Info.setRequiresImmediate(0, 63);
     return true;
   case 'K':
     Info.setRequiresImmediate(-128, 127);
     return true;
   case 'L':
-    // FIXME: properly analyze this constraint:
-    //  must be one of 0xff, 0xffff, or 0xffffffff
+    Info.setRequiresImmediate({ int(0xff), int(0xffff), int(0xffffffff) });
     return true;
   case 'M':
     Info.setRequiresImmediate(0, 3);
     return true;
   case 'N':
     Info.setRequiresImmediate(0, 255);
     return true;
   case 'O':
     Info.setRequiresImmediate(0, 127);
     return true;
-  case 'Y': // first letter of a pair:
-    switch (*(Name+1)) {
-    default: return false;
-    case '0':  // First SSE register.
-    case 't':  // Any SSE register, when SSE2 is enabled.
-    case 'i':  // Any SSE register, when SSE2 and inter-unit moves enabled.
-    case 'm':  // any MMX register, when inter-unit moves enabled.
-      break;   // falls through to setAllowsRegister.
+  // Register constraints.
+  case 'Y': // 'Y' is the first character for several 2-character constraints.
+    // Shift the pointer to the second character of the constraint.
+    Name++;
+    switch (*Name) {
+    default:
+      return false;
+    case '0': // First SSE register.
+    case 't': // Any SSE register, when SSE2 is enabled.
+    case 'i': // Any SSE register, when SSE2 and inter-unit moves enabled.
+    case 'm': // Any MMX register, when inter-unit moves enabled.
+      Info.setAllowsRegister();
+      return true;
     }
-  case 'f': // any x87 floating point stack register.
+  case 'f': // Any x87 floating point stack register.
     // Constraint 'f' cannot be used for output operands.
     if (Info.ConstraintStr[0] == '=')
       return false;
-
     Info.setAllowsRegister();
     return true;
   case 'a': // eax.
   case 'b': // ebx.
   case 'c': // ecx.
   case 'd': // edx.
   case 'S': // esi.
   case 'D': // edi.
   case 'A': // edx:eax.
-  case 't': // top of floating point stack.
-  case 'u': // second from top of floating point stack.
+  case 't': // Top of floating point stack.
+  case 'u': // Second from top of floating point stack.
   case 'q': // Any register accessible as [r]l: a, b, c, and d.
   case 'y': // Any MMX register.
   case 'x': // Any SSE register.
   case 'Q': // Any register accessible as [r]h: a, b, c, and d.
   case 'R': // "Legacy" registers: ax, bx, cx, dx, di, si, sp, bp.
   case 'l': // "Index" registers: any general register that can be used as an
             // index in a base+index memory access.
     Info.setAllowsRegister();
     return true;
+  // Floating point constant constraints.
   case 'C': // SSE floating point constant.
   case 'G': // x87 floating point constant.
-  case 'e': // 32-bit signed integer constant for use with zero-extending
-            // x86_64 instructions.
-  case 'Z': // 32-bit unsigned integer constant for use with zero-extending
-            // x86_64 instructions.
     return true;
   }
 }
 
 bool X86TargetInfo::validateOutputSize(StringRef Constraint,
                                        unsigned Size) const {
   // Strip off constraint modifiers.
   while (Constraint[0] == '=' ||
bool X86TargetInfo::validateOperandSize(StringRef Constraint,
   default: break;
   case 'y':
     return Size <= 64;
   case 'f':
   case 't':
   case 'u':
     return Size <= 128;
   case 'x':
+    if (SSELevel >= AVX512F)
+      // 512-bit zmm registers can be used if target supports AVX512F.
+      return Size <= 512U;
+    else if (SSELevel >= AVX)
       // 256-bit ymm registers can be used if target supports AVX.
-    return Size <= (SSELevel >= AVX ? 256U : 128U);
+      return Size <= 256U;
+    return Size <= 128U;
+  case 'Y':
+    // 'Y' is the first character for several 2-character constraints.
+    switch (Constraint[1]) {
+    default: break;
+    case 'm':
+      // 'Ym' is synonymous with 'y'.
+      return Size <= 64;
+    case 'i':
+    case 't':
+      // 'Yi' and 'Yt' are synonymous with 'x' when SSE2 is enabled.
+      if (SSELevel >= AVX512F)
+        return Size <= 512U;
+      else if (SSELevel >= AVX)
+        return Size <= 256U;
+      return SSELevel >= SSE2 && Size <= 128U;
+    }
+
   }
 
   return true;
 }
 
 std::string
 X86TargetInfo::convertConstraint(const char *&Constraint) const {
   switch (*Constraint) {

lib/Sema/SemaStmtAsm.cpp

if (Info.allowsMemory() && !Info.allowsRegister()) {
                          << InputExpr->getSourceRange());
     } else if (Info.requiresImmediateConstant() && !Info.allowsRegister()) {
       if (!InputExpr->isValueDependent()) {
         llvm::APSInt Result;
         if (!InputExpr->EvaluateAsInt(Result, Context))
            return StmtError(
                Diag(InputExpr->getLocStart(), diag::err_asm_immediate_expected)
                 << Info.getConstraintStr() << InputExpr->getSourceRange());
-         if (Result.slt(Info.getImmConstantMin()) ||
-             Result.sgt(Info.getImmConstantMax()))
+         if (!Info.isValidAsmImmediate(Result))
            return StmtError(Diag(InputExpr->getLocStart(),
                                  diag::err_invalid_asm_value_for_constraint)
                             << Result.toString(10) << Info.getConstraintStr()
                             << InputExpr->getSourceRange());
       }
 
     } else {
       ExprResult Result = DefaultFunctionArrayLvalueConversion(Exprs[i]);

test/CodeGen/mult-alt-x86.c

 // CHECK: @single_x
 void single_x()
 {
   // CHECK: asm "foo $1,$0", "=x,x[[CLOBBERS]](double {{[a-zA-Z0-9@%]+}})
   asm("foo %1,%0" : "=x" (dout0) : "x" (din1));
 }
 
 // CHECK: @single_Y
-void single_Y0()
+void single_Y()
 {
-  // Y constraint currently broken.
+  // 'Y' constraint currently broken.
   //asm("foo %1,%0" : "=Y0" (mout0) : "Y0" (min1));
   //asm("foo %1,%0" : "=Yz" (mout0) : "Yz" (min1));
   //asm("foo %1,%0" : "=Yt" (mout0) : "Yt" (min1));
   //asm("foo %1,%0" : "=Yi" (mout0) : "Yi" (min1));
   //asm("foo %1,%0" : "=Ym" (mout0) : "Ym" (min1));
 }
 
 // CHECK: @single_I

 {
   // CHECK: asm "foo $1,$0", "=*m,K[[CLOBBERS]](i32* @mout0, i32 1)
   asm("foo %1,%0" : "=m" (mout0) : "K" (1));
 }
 
 // CHECK: @single_L
 void single_L()
 {
-  // CHECK: asm "foo $1,$0", "=*m,L[[CLOBBERS]](i32* @mout0, i32 1)
-  asm("foo %1,%0" : "=m" (mout0) : "L" (1));
+  // CHECK: asm "foo $1,$0", "=*m,L[[CLOBBERS]](i32* @mout0, i32 255)
+  asm("foo %1,%0" : "=m" (mout0) : "L" (0xff));
+  // CHECK: asm "foo $1,$0", "=*m,L[[CLOBBERS]](i32* @mout0, i32 65535)
+  asm("foo %1,%0" : "=m" (mout0) : "L" (0xffff));
+  // CHECK: asm "foo $1,$0", "=*m,L[[CLOBBERS]](i32* @mout0, i32 -1)
+  asm("foo %1,%0" : "=m" (mout0) : "L" (0xffffffff));
 }
 
 // CHECK: @single_M
 void single_M()
 {
   // CHECK: asm "foo $1,$0", "=*m,M[[CLOBBERS]](i32* @mout0, i32 1)
   asm("foo %1,%0" : "=m" (mout0) : "M" (1));
 }

test/Sema/inline-asm-validate-x86.c

void K(int i, int j) {
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "K"(AboveMax)); // expected-error{{value '128' out of range for constraint 'K'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "K"(96)); // expected-no-error
 }
 
+void L(int i, int j) {
+  static const int Invalid1 = 1;
+  static const int Invalid2 = 42;
+  static const int Valid1 = 0xff;
+  static const int Valid2 = 0xffff;
+  static const int Valid3 = 0xffffffff;
+  __asm__("xorl %0,%2"
+          : "=r"(i)
+          : "0"(i), "L"(j)); // expected-error{{constraint 'L' expects an integer constant expression}}
+  __asm__("xorl %0,%2"
+          : "=r"(i)
+          : "0"(i), "L"(Invalid1)); // expected-error{{value '1' out of range for constraint 'L'}}
+  __asm__("xorl %0,%2"
+          : "=r"(i)
+          : "0"(i), "L"(Invalid2)); // expected-error{{value '42' out of range for constraint 'L'}}
+  __asm__("xorl %0,%2"
+          : "=r"(i)
+          : "0"(i), "L"(Valid1)); // expected-no-error
+  __asm__("xorl %0,%2"
+          : "=r"(i)
+          : "0"(i), "L"(Valid2)); // expected-no-error
+  __asm__("xorl %0,%2"
+          : "=r"(i)
+          : "0"(i), "L"(Valid3)); // expected-no-error
+}
+
 void M(int i, int j) {
   static const int BelowMin = -1;
   static const int AboveMax = 4;
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "M"(j)); // expected-error{{constraint 'M' expects an integer constant expression}}
   __asm__("xorl %0,%2"
           : "=r"(i)