Page MenuHomePhabricator

Emit diagnostic if an inline asm constraint requires an immediate
ClosedPublic

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

Diff Detail

Repository
rL LLVM

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.Jun 5 2019, 1:14 AM

Is there anything blocking this part from being committed?

joerg added a comment.Jun 5 2019, 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.

void updated this revision to Diff 206582.Jun 25 2019, 10:17 PM

Expand the diagnostic to support all constraints that require immediates.

void edited the summary of this revision. (Show Details)Jun 25 2019, 10:17 PM

I added a new constraint type C_Immdediate which indicates that a constraint requires an immediate value.

Please take a look.

void retitled this revision from Emit diagnostic if inline asm "n" constraint isn't an immediate to Emit diagnostic if an inline asm constraint requires an immediate.Jun 25 2019, 10:20 PM
void added a comment.Jul 9 2019, 12:08 AM

Friendly ping.

void updated this revision to Diff 210708.Jul 18 2019, 5:16 PM

Updating.

void updated this revision to Diff 210710.Jul 18 2019, 5:19 PM

Revert the prvious update

void updated this revision to Diff 210713.Jul 18 2019, 5:26 PM

Revert accidentally added files.

joerg added inline comments.Fri, Jul 26, 7:42 AM
lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
439 ↗(On Diff #210713)

This is an unrelated chunk. Can you drop it please? I like the idea, but it would be nicer if it didn't need to create an intermediate label at all.

void updated this revision to Diff 212029.Fri, Jul 26, 5:18 PM
void marked an inline comment as done.

Remove file that sneaked in.

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
439 ↗(On Diff #210713)

Ah! Yes, this sneaked in there...

With the exception of the small improvements outlined, this looks good to me.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7966 ↗(On Diff #212029)

Unrelated, but shouldn't Flags be filtered first? E.g.

Flags &= ~(InlineAsm::Extra_MayLoad | InlineAsm::Extra_MayStore);

There seems to be one test case (PowerPC/xray-ret-is-terminate.ll) that depends on this somewhat.

Adding C_Immediate here seems to be wrong though, since immediates are never loads or stores. As such, shall we just drop this chunk and bring up the Flags update separate?

8056 ↗(On Diff #212029)

like the "n" constraint

lib/Target/X86/X86ISelLowering.cpp
44780 ↗(On Diff #212029)

'L' means 0xFF or 0xFFFF, that should be C_Immediate.

44781 ↗(On Diff #212029)

'M' means 0..3 as shifts for LEA, should be C_Immediate as well.

void updated this revision to Diff 212068.Sat, Jul 27, 1:03 PM
void marked 4 inline comments as done.

Mark some constraints which require immediates as needing immediates.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7966 ↗(On Diff #212029)

I wanted to retain the original behavior, but yeah it seems like C_Immediate isn't really worth checking here.

As for the Flags, wouldn't clearing the flags remove previous iterations which set them? This is being called in a loop over all asm contraints...

joerg accepted this revision.Sat, Jul 27, 1:11 PM

LGTM otherwise.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8055 ↗(On Diff #212068)

Typo still.

This revision is now accepted and ready to land.Sat, Jul 27, 1:11 PM
void marked an inline comment as done.Sat, Jul 27, 1:18 PM
void updated this revision to Diff 212069.Sat, Jul 27, 1:18 PM

"like" :: "line" -- "potAYto" :: "potAHto"

:-)

void updated this revision to Diff 212071.Sat, Jul 27, 2:13 PM

Emit an error for an out-of-range immediate.

void updated this revision to Diff 212086.Sun, Jul 28, 12:58 AM

Update testing. This adds support for RISC-V inline asm immediate constraints
so that we emit the same errors that the front-end would.

void added a comment.Sun, Jul 28, 12:59 AM

I've updated this patch to better coincide with D60943. It adds support for RISC-V inline asm immediate constraints (to make the errors match those of the front-end) and a few more tests. Please take another look, since these are non-trivial changes.

Thanks!

This revision was automatically updated to reflect the committed changes.