This is an archive of the discontinued LLVM Phabricator instance.

Emit ASM input in a constant context
ClosedPublic

Authored by void on Dec 12 2018, 1:58 PM.

Details

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.

Diff Detail

Event Timeline

void created this revision.Dec 12 2018, 1:58 PM

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

efriedma added inline comments.
lib/CodeGen/CGStmt.cpp
1825

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 updated this revision to Diff 177936.Dec 12 2018, 2:42 PM
void marked an inline comment as done.

Don't look at the optimization level here.

void added a comment.Dec 12 2018, 2:43 PM

Addressed Eli's comment.

rehana added a subscriber: rehana.Dec 12 2018, 2:45 PM

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 updated this revision to Diff 178158.Dec 13 2018, 4:11 PM

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 added a comment.Dec 13 2018, 4:12 PM

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 added a comment.Dec 17 2018, 1:08 PM

Ping? :-)

efriedma added inline comments.Dec 17 2018, 1:45 PM
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 marked an inline comment as done.Dec 17 2018, 3:09 PM
void added inline comments.
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 accepted this revision.Dec 17 2018, 5:36 PM

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.

This revision is now accepted and ready to land.Dec 17 2018, 5:36 PM
void updated this revision to Diff 178582.Dec 17 2018, 8:10 PM

Reduce duplication by using multiple check prefixes.

void marked an inline comment as done.Dec 17 2018, 8:10 PM
efriedma added inline comments.Dec 18 2018, 12:11 PM
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 marked an inline comment as done.Dec 18 2018, 2:55 PM
void added inline comments.
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 updated this revision to Diff 178793.Dec 18 2018, 2:56 PM

Fix how prefixes are used in the testcase.

This revision was automatically updated to reflect the committed changes.

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 added a comment.Dec 18 2018, 9:02 PM

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!

@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?

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

void added a comment.Jan 11 2019, 5:57 PM

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 added a subscriber: mgorny.Apr 16 2019, 12:22 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 12:22 AM

@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