Emit ASM input in a constant context

[void]

Summary

Some ASM input constraints (e.g., "i" and "n") require immediate values. At O0,
very few code transformations are performed. So if we cannot resolve to an
immediate when emitting the ASM input we shouldn't delay its processing.

[void]

As a side note, the number of ways to evaluate a constant expression are legion in clang. They should really be unified...

[efriedma]

lib/CodeGen/CGStmt.cpp

1830

Checking the optimization level here doesn't make sense. If a value is required to be constant in some context, we should evaluate it the same way at all optimization levels.

It's also important that the evaluation here computes the same result as Sema::ActOnGCCAsmStmt; otherwise you could hit an assertion failure.

[void]

Don't look at the optimization level here.

[void]

Addressed Eli's comment.

[jyknight]

This seems like a good start, but not complete.

"n" and "i" both should require that their argument is a constant expression. For "n", it actually must be an immediate constant integer, so setRequiresImmediate() should be used there. For "i", you may use an lvalue constant as well. The way we seem to indicate that, today, is with if (!Info.allowsRegister() && !Info.allowsMemory()). However the code in that block does not today *require* that the evaluation as a constant succeed...and it should.

It should also only require that the result is an integer when requiresImmediateConstant().

Additionally, the Sema code needs to have the same conditions as the CodeGen code for when a constant expression is required, but it doesn't. (before or after your change).

[void]

The n constriant *requires* a known integer. Therefore, enforce this in both
Sema and CodeGen by setting the "requires immediate" flag and evaluating to a
known integer instead of random "int"..

This doesn't so much address i, which has other semantics. However, it
shouldn't regress how the i constraint is currently used.

[void]

In D55616#1329301, @jyknight wrote:

This seems like a good start, but not complete.

"n" and "i" both should require that their argument is a constant expression. For "n", it actually must be an immediate constant integer, so setRequiresImmediate() should be used there. For "i", you may use an lvalue constant as well. The way we seem to indicate that, today, is with if (!Info.allowsRegister() && !Info.allowsMemory()). However the code in that block does not today *require* that the evaluation as a constant succeed...and it should.

It should also only require that the result is an integer when requiresImmediateConstant().

Additionally, the Sema code needs to have the same conditions as the CodeGen code for when a constant expression is required, but it doesn't. (before or after your change).

Have I mentioned how much I *love* inline asm? :-)

I think I've addressed all of your concerns. PTAL.

[void]

Ping? :-)

[efriedma]

test/CodeGen/builtin-constant-p.c

2

Given this code doesn't check the optimization level anymore, do you still need separate check prefixes for O2 and O0. Or if that doesn't work for everything, maybe you could share a check prefix for some of the tests? (Maybe it would make sense to check IR generated using -disable-llvm-passes.)

[void]

test/CodeGen/builtin-constant-p.c

2

The bug only triggered at O0, so I still want to test it without optimizations. Note that we do check optimization levels during code generation to determine if we should generate an is.constant intrinsic.

[efriedma]

LGTM with a minor nit, but give a couple days for @jyknight to add any more comments.

test/CodeGen/builtin-constant-p.c

2

You can use something like "-check-prefixes=CHECK,O0" to reduce the duplication.

[void]

Reduce duplication by using multiple check prefixes.

[efriedma]

test/CodeGen/builtin-constant-p.c

2

That doesn't pass.

More specifically, I was thinking something more along the lines of using "--check-prefixes=CHECK,O2" for the first run, and "--check-prefixes=CHECK,O0" for the second run; then you can use "CHECK:" for the common lines and "O2:"/"O0:" for the lines that are different.

[void]

test/CodeGen/builtin-constant-p.c

2

I think think that works. The thing is that there should rarely be common lines between O2 and O0. I understand what you're going for here, but I don't think it's beneficial to this testcase. However, I don't need to change all of the CHECKs either. I came up with a compromise...

[void]

Fix how prefixes are used in the testcase.

[vitalybuka]

Looks like it's broken by this patch

clang: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/lib/AST/ExprConstant.cpp:11055: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext &, SmallVectorImpl<clang::PartialDiagnosticAt> *) const: Assertion `Result && "Could not evaluate expression"' failed.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/9281/steps/check-clang%20msan/logs/stdio

[void]

In D55616#1335587, @vitalybuka wrote:

Looks like it's broken by this patch

clang: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/lib/AST/ExprConstant.cpp:11055: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext &, SmallVectorImpl<clang::PartialDiagnosticAt> *) const: Assertion `Result && "Could not evaluate expression"' failed.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/9281/steps/check-clang%20msan/logs/stdio

This should be fixed now. Sorry about the breakage!

[george.karpenkov]

@void @efriedma I can't build XNU anymore after this commit, with an error message:

osfmk/i386/genassym.c:298:2: error: value '18446743523953737728' out of range for constraint 'n'
        DECLAREULL("KERNEL_BASE", KERNEL_BASE);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
osfmk/i386/genassym.c:107:65: note: expanded from macro 'DECLAREULL'
        __asm("DEFINITION__define__" SYM ":\t .ascii \"%0\"" : : "n"  ((unsigned long long)(VAL)))
                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~

The error message looks wrong: the value is in range to fit in unsigned long long (but I'm not an inline assembly expert).

At a minimum, could we add a test case to this change to show in which cases an extra diagnostics would appear?

[george.karpenkov]

Apologies - the value seems to indeed overflow, but I'm still very confused how this was affected by this change.

[void]

Apologies - the value seems to indeed overflow, but I'm still very confused how this was affected by this change.

What's different is that setRequiresImmediate is being set on the constraint where before it wasn't. If no range values are supplied (like in this patch), it defaults to INT_MIN and INT_MAX. From GNU's documentation, n accepts an integer, which we're treating as signed.

[mgorny]

This change apparently introduced a regression: https://bugs.llvm.org/show_bug.cgi?id=41027

[krytarowski]

@void hi, this broke assembly code on NetBSD for various archs and blocks upgrade of the toolchain.

Could you please have a look? https://bugs.llvm.org/show_bug.cgi?id=41027

lib/Basic/TargetInfo.cpp

case '[': {
 
       Info.setTiedOperand(Index, OutputConstraints[Index]);
       break;
     }
     case '%': // commutative
       // FIXME: Fail if % is used with the last operand.
       break;
     case 'i': // immediate integer.
+      break;
     case 'n': // immediate integer with a known value.
+      Info.setRequiresImmediate();
       break;
     case 'I':  // Various constant constraints with target-specific meanings.
     case 'J':
     case 'K':
     case 'L':
     case 'M':
     case 'N':
     case 'O':

lib/CodeGen/CGStmt.cpp

 
 llvm::Value* CodeGenFunction::EmitAsmInput(
                                          const TargetInfo::ConstraintInfo &Info,
                                            const Expr *InputExpr,
                                            std::string &ConstraintStr) {
   // 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()) {
+      llvm::APSInt AsmConst = InputExpr->EvaluateKnownConstInt(getContext());
+      return llvm::ConstantInt::get(getLLVMContext(), AsmConst);
+    }
+
     Expr::EvalResult Result;
     if (InputExpr->EvaluateAsInt(Result, getContext()))
       return llvm::ConstantInt::get(getLLVMContext(), Result.Val.getInt());
-    assert(!Info.requiresImmediateConstant() &&
-           "Required-immediate inlineasm arg isn't constant?");
   }
 
   if (Info.allowsRegister() || !Info.allowsMemory())
     if (CodeGenFunction::hasScalarEvaluationKind(InputExpr->getType()))
       return EmitScalarExpr(InputExpr);
   if (InputExpr->getStmtClass() == Expr::CXXThisExprClass)
     return EmitScalarExpr(InputExpr);
   InputExpr = InputExpr->IgnoreParenNoopCasts(getContext());

lib/Sema/SemaStmtAsm.cpp

for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; i++) {
     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->EvaluateAsInt(EVResult, Context))
+        llvm::SmallVector<PartialDiagnosticAt, 1> Diags;
+        llvm::APSInt Result = InputExpr->EvaluateKnownConstInt(Context, &Diags);
+        if (!Diags.empty())
           return StmtError(
               Diag(InputExpr->getBeginLoc(), diag::err_asm_immediate_expected)
               << Info.getConstraintStr() << InputExpr->getSourceRange());
-        llvm::APSInt Result = EVResult.Val.getInt();
-         if (!Info.isValidAsmImmediate(Result))
-           return StmtError(Diag(InputExpr->getBeginLoc(),
-                                 diag::err_invalid_asm_value_for_constraint)
-                            << Result.toString(10) << Info.getConstraintStr()
-                            << InputExpr->getSourceRange());
       }
 
     } else {
       ExprResult Result = DefaultFunctionArrayLvalueConversion(Exprs[i]);
       if (Result.isInvalid())
         return StmtError();
 
       Exprs[i] = Result.get();

test/CodeGen/builtin-constant-p.c

-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O2 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O2 | FileCheck --check-prefix=O2 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O0 | FileCheck --check-prefix=O0 %s
 
 int a = 42;
 
 inline int bcp(int x) {
   return __builtin_constant_p(x);
 }
 
 /* --- Compound literals */
 
 struct foo { int x, y; };
 
 int y;
 struct foo f = (struct foo){ __builtin_constant_p(y), 42 };
 
 struct foo test0(int expr) {
-  // CHECK: define i64 @test0(i32 %expr)
-  // CHECK: call i1 @llvm.is.constant.i32(i32 %expr)
+  // O2: define i64 @test0(i32 %expr)
+  // O2: call i1 @llvm.is.constant.i32(i32 %expr)
   struct foo f = (struct foo){ __builtin_constant_p(expr), 42 };
   return f;
 }
 
 /* --- Pointer types */
 
 inline int test1_i(int *x) {
   return *x;
 }
 
 int test1() {
-  // CHECK: define i32 @test1
-  // CHECK: add nsw i32 %0, -13
-  // CHECK-NEXT: call i1 @llvm.is.constant.i32(i32 %sub)
+  // O2: define i32 @test1
+  // O2: add nsw i32 %0, -13
+  // O2-NEXT: call i1 @llvm.is.constant.i32(i32 %sub)
   return bcp(test1_i(&a) - 13);
 }
 
 int test2() {
-  // CHECK: define i32 @test2
-  // CHECK: ret i32 0
+  // O2: define i32 @test2
+  // O2: ret i32 0
   return __builtin_constant_p(&a - 13);
 }
 
 inline int test3_i(int *x) {
   return 42;
 }
 
 int test3() {
-  // CHECK: define i32 @test3
-  // CHECK: ret i32 1
+  // O2: define i32 @test3
+  // O2: ret i32 1
   return bcp(test3_i(&a) - 13);
 }
 
 /* --- Aggregate types */
 
 int b[] = {1, 2, 3};
 
 int test4() {
-  // CHECK: define i32 @test4
-  // CHECK: ret i32 0
+  // O2: define i32 @test4
+  // O2: ret i32 0
   return __builtin_constant_p(b);
 }
 
 const char test5_c[] = {1, 2, 3, 0};
 
 int test5() {
-  // CHECK: define i32 @test5
-  // CHECK: ret i32 0
+  // O2: define i32 @test5
+  // O2: ret i32 0
   return __builtin_constant_p(test5_c);
 }
 
 inline char test6_i(const char *x) {
   return x[1];
 }
 
 int test6() {
-  // CHECK: define i32 @test6
-  // CHECK: ret i32 0
+  // O2: define i32 @test6
+  // O2: ret i32 0
   return __builtin_constant_p(test6_i(test5_c));
 }
 
 /* --- Non-constant global variables */
 
 int test7() {
-  // CHECK: define i32 @test7
-  // CHECK: call i1 @llvm.is.constant.i32(i32 %0)
+  // O2: define i32 @test7
+  // O2: call i1 @llvm.is.constant.i32(i32 %0)
   return bcp(a);
 }
 
 /* --- Constant global variables */
 
 const int c = 42;
 
 int test8() {
-  // CHECK: define i32 @test8
-  // CHECK: ret i32 1
+  // O2: define i32 @test8
+  // O2: ret i32 1
   return bcp(c);
 }
 
 /* --- Array types */
 
 int arr[] = { 1, 2, 3 };
 const int c_arr[] = { 1, 2, 3 };
 
 int test9() {
-  // CHECK: define i32 @test9
-  // CHECK: call i1 @llvm.is.constant.i32(i32 %0)
+  // O2: define i32 @test9
+  // O2: call i1 @llvm.is.constant.i32(i32 %0)
   return __builtin_constant_p(arr[2]);
 }
 
 int test10() {
-  // CHECK: define i32 @test10
-  // CHECK: ret i32 1
+  // O2: define i32 @test10
+  // O2: ret i32 1
   return __builtin_constant_p(c_arr[2]);
 }
 
 int test11() {
-  // CHECK: define i32 @test11
-  // CHECK: ret i32 0
+  // O2: define i32 @test11
+  // O2: ret i32 0
   return __builtin_constant_p(c_arr);
 }
 
 /* --- Function pointers */
 
 int test12() {
-  // CHECK: define i32 @test12
-  // CHECK: ret i32 0
+  // O2: define i32 @test12
+  // O2: ret i32 0
   return __builtin_constant_p(&test10);
 }
 
 int test13() {
-  // CHECK: define i32 @test13
-  // CHECK: ret i32 1
+  // O2: define i32 @test13
+  // O2: ret i32 1
   return __builtin_constant_p(&test10 != 0);
 }
 
 typedef unsigned long uintptr_t;
 #define assign(p, v) ({ \
   uintptr_t _r_a_p__v = (uintptr_t)(v);                           \
   if (__builtin_constant_p(v) && _r_a_p__v == (uintptr_t)0) {     \
     union {                                                       \

 extern int test15_v;
 
 struct { const char *t; int a; } test15[] = {
     { "tag", __builtin_constant_p(test15_v) && !test15_v ? 1 : 0 }
 };
 
 extern char test16_v;
 struct { int a; } test16 = { __builtin_constant_p(test16_v) };
+
+extern unsigned long long test17_v;
+
+void test17() {
+  // O0: define void @test17
+  // O0: call void asm sideeffect "", {{.*}}(i32 -1) 
+  // O2: define void @test17
+  // O2: call void asm sideeffect "", {{.*}}(i32 -1) 
+  __asm__ __volatile__("" :: "n"( (__builtin_constant_p(test17_v) || 0) ? 1 : -1));
+}