Page MenuHomePhabricator

Emit diagnostic if inline asm "n" constraint isn't an immediate
Needs ReviewPublic

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

Details

Summary

An inline asm call can result in an immediate after inlining. Therefore emit a
diagnostic here if the "n" constraint doesn't have an immediate.

Diff Detail

Event Timeline

void created this revision.Apr 21 2019, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2019, 1:36 AM
void updated this revision to Diff 195995.Apr 21 2019, 3:21 AM

Fix testcase to get the correct error message.

void updated this revision to Diff 196025.Apr 21 2019, 5:18 PM

Add testcase.

I can confirm that it fixes our issue but I don't feel confident reviewing the code itself. Copying reviewers from the original diff.

joerg added a comment.Apr 24 2019, 1:40 PM

It does not fix the issues on our side, but pushes them to a different place. It is still an improvement, but the problem is not solved yet.

void added a comment.May 13 2019, 4:42 PM

It does not fix the issues on our side, but pushes them to a different place. It is still an improvement, but the problem is not solved yet.

Joerg, What's the error again? (I forgot, sorry.)

joerg added a comment.May 13 2019, 4:54 PM

On the frontend side:

  • __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
  • The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

On the backend side:

  • The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

All that said, with the discussed generalization from literal 'n' to 'expects immediate', I think this code solves the part of the problem it is meant to solve.

void added a comment.May 13 2019, 5:22 PM

On the frontend side:

  • __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
  • The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

On the backend side:

  • The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

All that said, with the discussed generalization from literal 'n' to 'expects immediate', I think this code solves the part of the problem it is meant to solve.

W00T!

joerg added a comment.May 13 2019, 6:08 PM

On the frontend side:

  • __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
  • The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

Yes, that's correct. The problem is that with -O0, we never run SimplifyCFG, so those conditional branches make it straight to the backend. As discussed with Richard on IRC, the core issue is that for -O0, the IR generation for __builtin_constant_p is different from the constant folding as part of AST/EvalConstant.cpp. The latter is used by EmitIfStmt and friends to decide whether the BBs should be created in first place and currently fails evaluation for bcp.

On the backend side:

  • The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

With -O0, the intrinsic is never generated and no SimplifyCFG is run either (see above).

void added a comment.May 13 2019, 11:15 PM

On the frontend side:

  • __builtin_constant_p is not expanded correctly in EvalConstant.cpp when used as part of codegen for -O0. This means that trivially unreachable branches are emitted (e.g. with a BR with a literal false as condition),
  • The constant evaluator has a general problem with overly pessimistic analysis when it comes to possible side effects. Still have to discuss the correct handling with Richard for this.

GCC has this to say about __builtin_constant_p's behavior:

A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

When I was modifying our implementation recently, I made it so that if there are no optimizations then we evaluate to 0 in the front-end before code-gen. It might be that those blocks are being emitted, but they should have something like br i1 false, label %true_bb, label %false_bb or equivalent.

Yes, that's correct. The problem is that with -O0, we never run SimplifyCFG, so those conditional branches make it straight to the backend. As discussed with Richard on IRC, the core issue is that for -O0, the IR generation for __builtin_constant_p is different from the constant folding as part of AST/EvalConstant.cpp. The latter is used by EmitIfStmt and friends to decide whether the BBs should be created in first place and currently fails evaluation for bcp.

On the backend side:

  • The is_constant intrinsic is lowered too late. The prepare-isel pass it is currently part of is run after the last SimplifyCFG pass, so the now-dead basic blocks are never pruned and therefore the asm statements survive.

Is this at -O0 or all optimization levels?

With -O0, the intrinsic is never generated and no SimplifyCFG is run either (see above).

Ah! I understand now. I'll see if there's something I can do to help with this. Could you send me some test cases so that I can test things?

mgorny added a comment.Wed, Jun 5, 1:14 AM

Is there anything blocking this part from being committed?

joerg added a comment.Wed, Jun 5, 6:06 AM

The patch still needs to be generalized to handle all ICE constraints, but it is not blocked by the review for handling the is.constant intrinsic.