Page MenuHomePhabricator

Delay diagnosing asm constraints that require immediates until after inlining
ClosedPublic

Authored by void on Apr 21 2019, 1:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

void created this revision.Apr 21 2019, 1:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2019, 1:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
void updated this revision to Diff 195992.Apr 21 2019, 2:31 AM

Put constraint string check in the correct place.

void updated this revision to Diff 195994.Apr 21 2019, 3:08 AM

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

joerg added a comment.Apr 22 2019, 4:49 AM

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 added a comment.Apr 22 2019, 1:43 PM

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.

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 added a comment.Apr 22 2019, 5:31 PM

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.

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 added a comment.Apr 22 2019, 9:25 PM

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.

mgorny added a subscriber: mgorny.Apr 25 2019, 5:50 AM

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 updated this revision to Diff 206587.Jun 25 2019, 11:35 PM

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

void retitled this revision from Delay diagnosing "n" constraint until after inlining to Delay diagnosing asm constraints that require immediates until after inlining.Jun 25 2019, 11:37 PM

This should be ready for review now. PTAL.

void added a comment.Jul 9 2019, 12:08 AM

Friendly ping.

joerg added a comment.Jul 24 2019, 3:33 PM

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

void added a comment.Jul 26 2019, 5:13 PM

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

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

void updated this revision to Diff 212072.Jul 27 2019, 2:16 PM

Save checking of immediates until code generation.

void updated this revision to Diff 212079.Jul 27 2019, 11:38 PM

Don't emit errors here. Wait until codegen.

void updated this revision to Diff 212085.Jul 28 2019, 12:46 AM

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

void added a comment.Jul 31 2019, 2:30 PM

Friendly ping. :-)

joerg added a comment.Aug 2 2019, 5:30 AM

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

void added a comment.Aug 5 2019, 3:08 PM

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!

This revision was not accepted when it landed; it landed in state Needs Review.Aug 6 2019, 3:40 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 3:40 PM

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 added a comment.Aug 7 2019, 12:36 PM

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.