feat(parser): support forward references to SSA values#981
Conversation
There was a problem hiding this comment.
VeIR Benchmarks
Details
| Benchmark suite | Current: 22378ee | Previous: a8086a5 | Ratio |
|---|---|---|---|
add-fold-worklist/create |
2106000 ns (± 29368) |
2191000 ns (± 116585) |
0.96 |
add-fold-worklist/rewrite |
3912000 ns (± 129177) |
4025500 ns (± 99281) |
0.97 |
add-fold-worklist-local/create |
2174000 ns (± 67284) |
2104000 ns (± 59879) |
1.03 |
add-fold-worklist-local/rewrite |
3311000 ns (± 40214) |
3305000 ns (± 90508) |
1.00 |
add-zero-worklist/create |
2184500 ns (± 318938) |
2234000 ns (± 109081) |
0.98 |
add-zero-worklist/rewrite |
2524000 ns (± 426501) |
2750000 ns (± 105299) |
0.92 |
add-zero-reuse-worklist/create |
1841500 ns (± 78624) |
1842000 ns (± 83972) |
1.00 |
add-zero-reuse-worklist/rewrite |
2084500 ns (± 97775) |
2160000 ns (± 52555) |
0.97 |
mul-two-worklist/create |
2104000 ns (± 73164) |
2231000 ns (± 55895) |
0.94 |
mul-two-worklist/rewrite |
5583000 ns (± 54410) |
5644500 ns (± 283151) |
0.99 |
add-fold-forwards/create |
2185000 ns (± 83948) |
2202000 ns (± 109662) |
0.99 |
add-fold-forwards/rewrite |
3011000 ns (± 32120) |
3032000 ns (± 65213) |
0.99 |
add-zero-forwards/create |
2190500 ns (± 168726) |
2265000 ns (± 96205) |
0.97 |
add-zero-forwards/rewrite |
1965500 ns (± 127586) |
1987500 ns (± 93854) |
0.99 |
add-zero-reuse-forwards/create |
1788000 ns (± 73761) |
1832000 ns (± 89057) |
0.98 |
add-zero-reuse-forwards/rewrite |
1522000 ns (± 49702) |
1542000 ns (± 22638) |
0.99 |
mul-two-forwards/create |
2117000 ns (± 39981) |
2198000 ns (± 41455) |
0.96 |
mul-two-forwards/rewrite |
3599000 ns (± 45098) |
3767000 ns (± 172147) |
0.96 |
add-zero-reuse-first/create |
1841500 ns (± 91463) |
1855500 ns (± 114257) |
0.99 |
add-zero-reuse-first/rewrite |
8000 ns (± 2236) |
8500 ns (± 2543) |
0.94 |
add-zero-lots-of-reuse-first/create |
1815000 ns (± 84382) |
1789000 ns (± 89329) |
1.01 |
add-zero-lots-of-reuse-first/rewrite |
808000 ns (± 27799) |
803000 ns (± 57817) |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Bad linearity, some fixes incoming. |
|
Nice!
Looking forward to them. |
|
yay!! psyched for this to land |
9955270 to
fa218be
Compare
3d13c8d to
a02d562
Compare
math-fehr
left a comment
There was a problem hiding this comment.
Nice! I added some comments inline, but otherwise I see two "bugs" here:
-
Right now, entry points in the parser (like
parseOp) might let the forward declared values escape. So for instance if we parse%x = my.op ..., then%xis forward declared, but the operation is still returned to the user. So we should check in all public operations that the forward declared values are all gone. -
This one is more subtle, but look at what happens when you parse
"builtin.module"() ({
"test.use"(%a) : (i32) -> ()
"test.wrapper"() ({
"test.use"(%a) : (i32) -> ()
%a = "test.def"() : () -> i32
}) : () -> ()
%a = "test.def"() : () -> i32
}) : () -> ()
Before, it was failing because a was redefined, but now it seems like it is making this program pass without an error (and also uses the second %a everywhere. Can you have this test and make sure it returns an error as well?
9ecb0a1 to
41b5c0f
Compare
Allow an SSA value to be used before its definition (a "forward reference").
When a value name is used before it is defined, a detached single-result
placeholder operation (`builtin.unrealized_conversion_cast`) stands in for it;
when the definition is parsed, every use of the placeholder is rewired to the
real value and the placeholder is erased.
Resolution follows MLIR's generic-form parser: the SSA name table is flat, so a
forward reference is resolved by the first textual definition of the name,
wherever it appears (including inside a nested region). Names defined in a scope
are dropped when that scope closes, so sibling scopes may reuse them.
Whether a resolved reference is actually legal (SSA dominance, IsolatedFromAbove)
is a verifier concern that MLIR checks after parsing; VeIR likewise defers it to
a later verification pass. The parser only reports values that are never defined
anywhere ("use of undefined value").
`parseTopLevelOp` wraps `parseOp` and performs the final unresolved-reference
check; all external entrypoints use it.
41b5c0f to
22378ee
Compare
| let parserState ← (ParserState.fromInput s.toByteArray).mapError toString | ||
| let (op, mlirState, _) ← | ||
| ((parseOp none).run (MlirParserState.fromContext ctx) parserState).mapError toString | ||
| (Veir.Parser.parseTopLevelOp.run (MlirParserState.fromContext ctx) parserState).mapError toString |
There was a problem hiding this comment.
Unfortunate name conflict :(
Implements MLIR parser support for SSA forward references.
The parser now:
builtin.unrealized_conversion_castoperation results as placeholders, rather than adding a new SSA value kindAdded coverage for three cases:
[]successor syntax