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 -O0 | FileCheck --check-prefixes=CHECK,O0 %s
 
 int a = 42;
 
 inline int bcp(int x) {
   return __builtin_constant_p(x);
 }
 
 /* --- Compound literals */

 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) 
+  // CHECK: define void @test17
+  // CHECK: call void asm sideeffect "", {{.*}}(i32 -1) 
+  __asm__ __volatile__("" :: "n"( (__builtin_constant_p(test17_v) || 0) ? 1 : -1));
+}