Implement left-recursive rule optimization #1399
Conversation
| } | ||
|
|
||
| // Require all return states to return back to the same rule | ||
| // that p is in. |
There was a problem hiding this comment.
📝 We are eliminating lookahead for one outgoing edge. This will affect all of the ATN configurations represented by the prediction context graph, so we need to verify that all of the configurations are affected by the ambiguity, hence why we verify the target of all return states.
| if (i == 0 && p.getStateType() == ATNState.STAR_LOOP_ENTRY && ((StarLoopEntryState)p).isPrecedenceDecision) { | ||
| boolean suppress = !config.context.isEmpty(); | ||
| int limit = config.context.size(); | ||
| if (config.context.hasEmptyPath()) { |
There was a problem hiding this comment.
❗️ This is likely a bug. If we have an empty path we should automatically not suppress the edge.
sharwell
force-pushed
the
dyn-disambig
branch
from
7f3c714
to
c844680
Compare
| } | ||
|
|
||
| if (!suppress) { | ||
| break; |
There was a problem hiding this comment.
❗️ Should be the following (copy paste error):
if (suppress) {
continue;
}
| continue; | ||
| } | ||
|
|
||
| ATNState returnState = atn.states.get(config.context.getReturnState(j)); |
There was a problem hiding this comment.
💡 No need to get the ATNState if we just need the state number.
Fixes antlr#1398 Fixes antlr#994
sharwell
force-pushed
the
dyn-disambig
branch
from
c844680
to
69ad8c0
Compare
parrt
changed the title
| continue; | ||
| } | ||
|
|
||
| int returnStateNumber = config.context.getReturnState(j); |
There was a problem hiding this comment.
Don't bother fixing as I've pulled and factored into separate method...
| // Further check to make sure this isn't a 0-precedence entry. | ||
| // See antlr/antlr4#679. | ||
| if (suppress) { | ||
| RuleStopState ruleStopState = atn.ruleToStopState[p.ruleIndex]; |
There was a problem hiding this comment.
Can't we test something simpler here? Perhaps we verify that all returnStateNumber values are the same after checking they are in the same rule? Why bother with FOLLOW edges?
| continue; | ||
| } | ||
|
|
||
| if (((EpsilonTransition)transition).outermostPrecedenceReturn() != p.ruleIndex) { |
There was a problem hiding this comment.
Can you help me understand why precedence DFA stuff comes into play here?
There was a problem hiding this comment.
outermostPrecedenceReturn labels edges in the ATN which return from an LR rule invocation where the precedence was 0. All the return edges for non-zero precedence go to the block end state that we're looking for.
|
I'm closing after manually pulling most of this into my own version (that is easy for my brain to handle ha!). #1404 |
Fixes #1398
Fixes #994
📝 For #1323 I'm observing a 10,000:1 performance improvement. Yeah, that much! 🎉 /cc @sufail