Delay diagnosing asm constraints that require immediates until after inlining

[void]

Summary

An inline asm call may result in an immediate input value after inlining.
Therefore, don't emit a diagnostic here if the input isn't an immediate.

[void]

Put constraint string check in the correct place.

[void]

Fix test. Use "e" instead of "n".

[void]

This relies upon D60943.

[joerg]

I'm in the process of testing this, but feedback will take a bit.

On the more meaty parts of this change, I think further iterations will be necessary in-tree to extend this to the other constraints.

[void]

In D60943#1474091, @joerg wrote:

I'm in the process of testing this, but feedback will take a bit.

On the more meaty parts of this change, I think further iterations will be necessary in-tree to extend this to the other constraints.

Thanks, Joerg. I was wondering about other constraints. This can be easily extended to encompass them. I just didn't want to jump the gun and come up with a "fix" that wasn't necessary.

[rsmith]

I don't like making the validity of an input depend on optimizations, but I presume this is necessary to build Linux or similar? I'd be more comfortable with this if we still did the eager check if the enclosing function doesn't have the always_inline attribute. Is that sufficient to satisfy the use cases?

[void]

Here's the motivating bug report: https://bugs.llvm.org/show_bug.cgi?id=41027

In general, I agree with you that diagnostics shouldn't depend on optimization levels, but inline assembly subverts this paradigm because it was originally a gcc extension. :-( That said, I don't know if looking at the always_inline attribute would work. I'll let @joerg and others comment on that.

[rsmith]

In D60943#1474899, @void wrote:

Here's the motivating bug report: https://bugs.llvm.org/show_bug.cgi?id=41027

Thanks, that's illuminating. OK, if we want to support that code, then there's really not much validation we can do in the frontend for constraints that require immediate constants. Is "n" really special in this regard, or is it just the first one that we've encountered?

In general, I agree with you that diagnostics shouldn't depend on optimization levels, but inline assembly subverts this paradigm because it was originally a gcc extension. :-(

There is a question of how much weird stuff we should accept in the name of GCC compatibility. In this case, the fact that the frontend rejects seems like the tip of an iceberg: the code in the PR is also relying on the control flow of the && inside the if to be emitted in a very particular way (or to be optimized to that form), and for dead-code elimination to be considered and to actually fire, and probably more things besides. Do we at least still produce a nice diagnostic if the value ends up not being a constant?

[void]

In D60943#1474926, @rsmith wrote:

In D60943#1474899, @void wrote:

Here's the motivating bug report: https://bugs.llvm.org/show_bug.cgi?id=41027

Thanks, that's illuminating. OK, if we want to support that code, then there's really not much validation we can do in the frontend for constraints that require immediate constants. Is "n" really special in this regard, or is it just the first one that we've encountered?

I think it's just the first one we've encountered. There could be other constraints which require immediates, but they're probably target-specific. E.g. e and K for x86.

In general, I agree with you that diagnostics shouldn't depend on optimization levels, but inline assembly subverts this paradigm because it was originally a gcc extension. :-(

There is a question of how much weird stuff we should accept in the name of GCC compatibility. In this case, the fact that the frontend rejects seems like the tip of an iceberg: the code in the PR is also relying on the control flow of the && inside the if to be emitted in a very particular way (or to be optimized to that form), and for dead-code elimination to be considered and to actually fire, and probably more things besides. Do we at least still produce a nice diagnostic if the value ends up not being a constant?

It does, but for the wrong line:

[morbo@fawn:llvm] cat bad.c
static __attribute__((always_inline)) void outl(unsigned port, unsigned data) {
  asm volatile("outl %0,%w1" : : "a"(data), "n"(port));
}

void f(unsigned port) {
  outl(port, 1);
}
[morbo@fawn:llvm] ./llvm.opt.install/bin/clang -c bad.c
bad.c:2:16: error: constraint 'n' expects an integer constant expression
  asm volatile("outl %0,%w1" : : "a"(data), "n"(port));
               ^
1 error generated.

I'll need to modify the LLVM patch to output the correct location.

[rsmith]

In D60943#1475038, @void wrote:

In D60943#1474926, @rsmith wrote:

Is "n" really special in this regard, or is it just the first one that we've encountered?

I think it's just the first one we've encountered. There could be other constraints which require immediates, but they're probably target-specific. E.g. e and K for x86.

OK, then I think we should apply whatever we do here to all constraints for which Info.requiresImmediateConstant() is true.

[void]

Modify so that no diagnistics are emitted until we can determine if it's an
immediate or not.

[void]

This should be ready for review now. PTAL.

[void]

Friendly ping.

[joerg]

You lost the changes to lib/Sema/SemaStmtAsm.cpp to actually do the delaying for immediate operands?

[void]

That was intentional. We want to do this for more than just the n constraint.

[joerg]

I understand, but the current version just doesn't work anyway to delay it.

[void]

Save checking of immediates until code generation.

[void]

Don't emit errors here. Wait until codegen.

[void]

Move a few more tests around. Some go to the LLVM side.

[void]

Friendly ping. :-)

[joerg]

The combination of D60942, D06943 and D65280 solves the problems for me on all targets I have.

[void]

In D60943#1612083, @joerg wrote:

The combination of D60942, D06943 and D65280 solves the problems for me on all targets I have.

Excellent! I submitted D60942. Go ahead and LGTM please. :-) Thanks!

[JamesNagurne]

In case you haven't seen, this commit breaks non-x86 build bots due to the combination of '-triple x86_64*' and '-S'. Some tests that use this target are only looking for AST dumps, and do not actually require such a target. This is not one of those tests, as it's inspecting assembly.
See clang/test/CodeGen/addrsig.c to see how that is handled (via REQUIRES: x86-registered-target).

Oddly enough, other tests that use -triple x86_64* and only inspect the AST don't require the registered target.

[void]

In D60943#1619579, @JamesNagurne wrote:

In case you haven't seen, this commit breaks non-x86 build bots due to the combination of '-triple x86_64*' and '-S'. Some tests that use this target are only looking for AST dumps, and do not actually require such a target. This is not one of those tests, as it's inspecting assembly.
See clang/test/CodeGen/addrsig.c to see how that is handled (via REQUIRES: x86-registered-target).

Oddly enough, other tests that use -triple x86_64* and only inspect the AST don't require the registered target.

Sorry about that. Fixed in r368202.

lib/CodeGen/CGStmt.cpp

llvm::Value* CodeGenFunction::EmitAsmInput(
   // If this can't be a register or memory, i.e., has to be a constant
   // (immediate or symbolic), try to emit it as such.
   if (!Info.allowsRegister() && !Info.allowsMemory()) {
     if (Info.requiresImmediateConstant()) {
       Expr::EvalResult EVResult;
       InputExpr->EvaluateAsRValue(EVResult, getContext(), true);
 
       llvm::APSInt IntResult;
-      if (!EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
-                                           getContext()))
-        llvm_unreachable("Invalid immediate constant!");
-
+      if (EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
         return llvm::ConstantInt::get(getLLVMContext(), IntResult);
     }
 
     Expr::EvalResult Result;
     if (InputExpr->EvaluateAsInt(Result, getContext()))
       return llvm::ConstantInt::get(getLLVMContext(), Result.Val.getInt());
   }
 
   if (Info.allowsRegister() || !Info.allowsMemory())

lib/Sema/SemaStmtAsm.cpp

if (Info.allowsMemory() && !Info.allowsRegister()) {
       if (CheckAsmLValue(InputExpr, *this))
         return StmtError(Diag(InputExpr->getBeginLoc(),
                               diag::err_asm_invalid_lvalue_in_input)
                          << Info.getConstraintStr()
                          << InputExpr->getSourceRange());
     } else if (Info.requiresImmediateConstant() && !Info.allowsRegister()) {
       if (!InputExpr->isValueDependent()) {
         Expr::EvalResult EVResult;
-        if (!InputExpr->EvaluateAsRValue(EVResult, Context, true))
-          return StmtError(
-              Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
-              << Info.getConstraintStr() << InputExpr->getSourceRange());
-
+        if (InputExpr->EvaluateAsRValue(EVResult, Context, true)) {
           // For compatibility with GCC, we also allow pointers that would be
           // integral constant expressions if they were cast to int.
           llvm::APSInt IntResult;
-        if (!EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
+          if (EVResult.Val.toIntegralConstant(IntResult, InputExpr->getType(),
                                                Context))
-          return StmtError(
-              Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
-              << Info.getConstraintStr() << InputExpr->getSourceRange());
-
             if (!Info.isValidAsmImmediate(IntResult))
               return StmtError(Diag(InputExpr->getBeginLoc(),
                                     diag::err_invalid_asm_value_for_constraint)
-                           << IntResult.toString(10) << Info.getConstraintStr()
+                               << IntResult.toString(10)
+                               << Info.getConstraintStr()
                                << InputExpr->getSourceRange());
         }
+      }
 
     } else {
       ExprResult Result = DefaultFunctionArrayLvalueConversion(Exprs[i]);
       if (Result.isInvalid())
         return StmtError();
 
       Exprs[i] = Result.get();
     }

test/CodeGen/pr41027.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -O2 -o - %s | FileCheck %s
+
+// CHECK-LABEL: f:
+// CHECK:         movl $1, %eax
+// CHECK-NEXT:    #APP
+// CHECK-NEXT:    outl %eax, $1
+// CHECK-NEXT:    #NO_APP
+
+static inline void pr41027(unsigned a, unsigned b) {
+  if (__builtin_constant_p(a)) {
+    __asm__ volatile("outl %0,%w1" : : "a"(b), "n"(a));
+  } else {
+    __asm__ volatile("outl %0,%w1" : : "a"(b), "d"(a));
+  }
+}
+
+void f(unsigned port) {
+  pr41027(1, 1);
+}

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

 // RUN: %clang_cc1 -triple riscv32 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple riscv64 -fsyntax-only -verify %s
 
 void I(int i) {
   static const int BelowMin = -2049;
   static const int AboveMax = 2048;
-  asm volatile ("" :: "I"(i)); // expected-error{{constraint 'I' expects an integer constant expression}}
   asm volatile ("" :: "I"(BelowMin)); // expected-error{{value '-2049' out of range for constraint 'I'}}
   asm volatile ("" :: "I"(AboveMax)); // expected-error{{value '2048' out of range for constraint 'I'}}
 }
 
 void J(int j) {
   static const int BelowMin = -1;
   static const int AboveMax = 1;
-  asm volatile ("" :: "J"(j)); // expected-error{{constraint 'J' expects an integer constant expression}}
   asm volatile ("" :: "J"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'J'}}
   asm volatile ("" :: "J"(AboveMax)); // expected-error{{value '1' out of range for constraint 'J'}}
 }
 
 void K(int k) {
   static const int BelowMin = -1;
   static const int AboveMax = 32;
-  asm volatile ("" :: "K"(k)); // expected-error{{constraint 'K' expects an integer constant expression}}
   asm volatile ("" :: "K"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'K'}}
   asm volatile ("" :: "K"(AboveMax)); // expected-error{{value '32' out of range for constraint 'K'}}
 }

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

 // RUN: %clang_cc1 -triple i686 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify -DAMD64 %s
 
 void I(int i, int j) {
   static const int BelowMin = -1;
   static const int AboveMax = 32;
   __asm__("xorl %0,%2"
           : "=r"(i)
-          : "0"(i), "I"(j)); // expected-error{{constraint 'I' expects an integer constant expression}}
-  __asm__("xorl %0,%2"
-          : "=r"(i)
           : "0"(i), "I"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'I'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "I"(AboveMax)); // expected-error{{value '32' out of range for constraint 'I'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "I"(16)); // expected-no-error
 }
 
 void J(int i, int j) {
   static const int BelowMin = -1;
   static const int AboveMax = 64;
   __asm__("xorl %0,%2"
           : "=r"(i)
-          : "0"(i), "J"(j)); // expected-error{{constraint 'J' expects an integer constant expression}}
-  __asm__("xorl %0,%2"
-          : "=r"(i)
           : "0"(i), "J"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'J'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "J"(AboveMax)); // expected-error{{value '64' out of range for constraint 'J'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "J"(32)); // expected-no-error
 }
 
 void K(int i, int j) {
   static const int BelowMin = -129;
   static const int AboveMax = 128;
   __asm__("xorl %0,%2"
           : "=r"(i)
-          : "0"(i), "K"(j)); // expected-error{{constraint 'K' expects an integer constant expression}}
-  __asm__("xorl %0,%2"
-          : "=r"(i)
           : "0"(i), "K"(BelowMin)); // expected-error{{value '-129' out of range for constraint 'K'}}
   __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 Invalid3 = 0;
   static const long long Invalid4 = 0x1000000ff;
   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"(Invalid3)); // expected-error{{value '0' out of range for constraint 'L'}}
   __asm__("xorl %0,%2"
__asm__("xorl %0,%2"
           : "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)
           : "0"(i), "M"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'M'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "M"(AboveMax)); // expected-error{{value '4' out of range for constraint 'M'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "M"(2)); // expected-no-error
 }
 
 void N(int i, int j) {
   static const int BelowMin = -1;
   static const int AboveMax = 256;
   __asm__("xorl %0,%2"
           : "=r"(i)
-          : "0"(i), "N"(j)); // expected-error{{constraint 'N' expects an integer constant expression}}
-  __asm__("xorl %0,%2"
-          : "=r"(i)
           : "0"(i), "N"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'N'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "N"(AboveMax)); // expected-error{{value '256' out of range for constraint 'N'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "N"(128)); // expected-no-error
 }
 
 void O(int i, int j) {
   static const int BelowMin = -1;
   static const int AboveMax = 128;
   __asm__("xorl %0,%2"
           : "=r"(i)
-          : "0"(i), "O"(j)); // expected-error{{constraint 'O' expects an integer constant expression}}
-  __asm__("xorl %0,%2"
-          : "=r"(i)
           : "0"(i), "O"(BelowMin)); // expected-error{{value '-1' out of range for constraint 'O'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "O"(AboveMax)); // expected-error{{value '128' out of range for constraint 'O'}}
   __asm__("xorl %0,%2"
           : "=r"(i)
           : "0"(i), "O"(64)); // expected-no-error
 }
 
 void pr40890(void) {
   struct s {
     int a, b;
   };
   static struct s s;
   // This null pointer can be used as an integer constant expression.
   __asm__ __volatile__("\n#define S_A abcd%0\n" : : "n"(&((struct s*)0)->a));
   // This offset-from-null pointer can be used as an integer constant expression.
   __asm__ __volatile__("\n#define S_B abcd%0\n" : : "n"(&((struct s*)0)->b));
-  // This pointer cannot be used as an integer constant expression.
-  __asm__ __volatile__("\n#define GLOBAL_A abcd%0\n" : : "n"(&s.a)); // expected-error{{constraint 'n' expects an integer constant expression}}
-  // Floating-point is also not okay.
-  __asm__ __volatile__("\n#define PI abcd%0\n" : : "n"(3.14f)); // expected-error{{constraint 'n' expects an integer constant expression}}
 #ifdef AMD64
   // This arbitrary pointer is fine.
   __asm__ __volatile__("\n#define BEEF abcd%0\n" : : "n"((int*)0xdeadbeeeeeef));
 #endif
 }

test/Sema/pr41027.c

-// RUN: %clang_cc1 -triple x86_64 -fsyntax-only %s
-// XFAIL: *
-
-inline void pr41027(unsigned a, unsigned b) {
-  if (__builtin_constant_p(a)) {
-    __asm__ volatile("outl %0,%w1" : : "a"(b), "n"(a));
-  } else {
-    __asm__ volatile("outl %0,%w1" : : "a"(b), "d"(a));
-  }
-}