Extend & fix FunctionInliner

[asl]

• Ensure it really can do multiple iterations via clearing stale worklist
• Allow inlining for more complex cases: LHS of assignment and variable initializers

Some cleanup while there

Fixes #4796

[asl]

This PR supersedes #4800 I made it on top of it, so the difference in the test output could be seem from the top commit (5c1e37f#diff-ff529ab5b2ef38af72effa10270da859185b7a906f3e49d50dfbf4db7d790f5e).

It turned out that FunctionInliner was not quite able to made multiple iterations as stale entries were left on the worklist and they effectively prevented the second iteration. Also, it was not able to deal with the things it created by itself during inlining – variable declarations (for arguments) that contained calls to be inlined.

[mihaibudiu]

I can't say that I remember this code very well, but this seems fine.

[mihaibudiu]

this could be probably smaller to highlight just the problem.
you don't need to use pna.p4

[asl]

Yeah, right. Just wanted to add exactly the testcase from the issue.

[mihaibudiu]

I am assuming this requires a block expression to be legal in the calling context.
is this always true?

[asl]

I might be missing something, but it seems to be so:

• DiscoverFunctionsInlining would collect Declaration_Variable's that have MethodCallExpression inside the initializer and only those.
• In fact, at this time the only cases for such Declaration_Variable's are the ones that were created by functionInliner by itself on the earlier iteration (for function arguments), everything else is already replaced by initializer-less ones, plus explicit AssignmentStatement.

Are there any other cases to consider?

[mihaibudiu]

the only thing I can think of are constant declarations.
If a constant initializer can involve action calls, it may be a problem.

[jafingerhut]

I have difficulty imagining how a constant initializer could involve an action call, since actions calls are not expressions that return values. Thus I believe they cannot be part of an expression, only a full statement by themselves.

I constant initializer could involve function calls, of course. Not sure if that is relevant here.

[asl]

constants are Declaration_Constant, so they are excluded here. And per spec:

Variables declarations can appear in the following locations within a P4 program:

 * In a block statement,
 * In a parser state,
 * In an action body,
 * In a control block's apply sub-block,
 * In the list of local declarations in a parser, and
 * In the list of local declarations in a control.

So, I think block expression is legal in all these places.

[mihaibudiu]

For now I think you can leave it as it is. If somebody complains you have a hypothesis.
Or you can try to write a test to see if you can force declarations in this position. Maybe it's impossible. Check the shape of the program before this pass is run.

[asl]

I mean, if there is an invariant that variable initializers are already de-sugared at this stage, then we should not create new ones, right? This would actually make Declaration_Variable case here unused as everything will be handled by the existing AssignmentStatement handle. This probably will be cleaner solution, let me try this.

[mihaibudiu]

I think that the MoveDeclarations is the pass which does this.

[ChrisDodd]

Yes, MoveDeclarations does this.

[asl]

Ok, I went ahead and just create uninitialized variable there. It seems that existing AssignmentStatement case perfectly handles it, no need for something additional.