Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a frontend policy as a customization point for frontend #4406

Merged
merged 8 commits into from
Feb 13, 2024

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Feb 9, 2024

Breaking change (dev/test): Side-effects ordering is enabled in frontend tests. This was done by mistake here, but it is probably a good thing, see #4433 for more details.

This was spurred by #4402, but the scope is a bit more general. I've tried to allow customization (almost) all the customizations of frontend into one class that a backend tool can override to tweak certain behaviours.

On top of the things that were already there, and the customization of constant folding that @grg had in #4402, I've added a hook for deciding if optimizations should run -- that is a use case that I have.

Copy link
Contributor

@ChrisDodd ChrisDodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good (probably better than what is in #4402, and would make that unnecessary).

Part of the issue is that the frontend ends up doing too much -- it really should not be doing inlining at all and probably should not be doing constant folding, except to the extent that it is needed for type checking and inferencing. That's really the sticking point, as we need to fold expressions used in type contexts (eg, in the N in bit<N> and int<N>), which may also require some inlining to squeeze everything down to a constant. I'm not sure what the right answer is.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 12, 2024

This looks good (probably better than what is in #4402, and would make that unnecessary).

Thank you.

Part of the issue is that the frontend ends up doing too much -- it really should not be doing inlining at all and probably should not be doing constant folding, except to the extent that it is needed for type checking and inferencing. That's really the sticking point, as we need to fold expressions used in type contexts (eg, in the N in bit<N> and int<N>), which may also require some inlining to squeeze everything down to a constant. I'm not sure what the right answer is.

I agree. A lot can be moved to midend, but we need to be able to type-check in frontend. But apparently we do run type checking before constant folding so maybe it is not needed as much? Mauybe this is somewhat related to @jnfoster's p4lang/p4-spec#1213, but haven't read through that proposal.

The p4c frontend does a lot of things that would be considered "midend" by many other compilers. And it seems like you are hitting cases where the frontend is not universal and applicable to all the p4-based tools (it would be quite interesting to know a bit more on what they are, if that can be said publicly). Thinking about it, p4Tools/p4Testgen could probably also benefit from having non-inlined code -- there would be some cost of course, but at least it would produce more human-readable traces.

I think the first step towards a solution would be to describe what the current frontend does. Just at a glance, we might want to split it into a frontend (that basically just checks the P4 is valid and inserts things like implicit cast) and a "flattener" that produces a flat P4 code without function, parser and control calls (with table / action and extern calls only). But the main probably could be that if someone skips the flattening, then a lot of the midend passes would not work properly.

Copy link
Contributor

@grg grg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vlstill vlstill force-pushed the frontend-policy branch 2 times, most recently from 167f407 to 1e35b94 Compare February 12, 2024 19:16
@vlstill vlstill enabled auto-merge (squash) February 13, 2024 05:31
@vlstill vlstill merged commit 8a6bcda into p4lang:main Feb 13, 2024
16 checks passed
@vlstill vlstill deleted the frontend-policy branch February 13, 2024 07:09
@fruffy
Copy link
Collaborator

fruffy commented Feb 13, 2024

One thing we could is to at least start to remove passes from the front end. I think a lot of them have been added at random and may have little effect. Thankfully, we do have the reference files to make sure transformations are semantics-preserving.

Thinking about it, p4Tools/p4Testgen could probably also benefit from having non-inlined code -- there would be some cost of course, but at least it would produce more human-readable traces.

Yes, there will be quite a bit of work for that. We rely a lot on the control-flow optimizations of RemoveReturns and RemoveExit. This is only possible because of inlining.

grg added a commit that referenced this pull request Feb 14, 2024
Pass the ConstantFoldingPolicy instance to the ConstantFolding instance
in SpecializeAll. This was missed in #4406.
grg added a commit that referenced this pull request Feb 14, 2024
Pass the ConstantFoldingPolicy instance to the ConstantFolding instance
in SpecializeAll. This was missed in #4406.
vlstill pushed a commit that referenced this pull request Feb 14, 2024
Pass the ConstantFoldingPolicy instance to the ConstantFolding instance
in SpecializeAll. This was missed in #4406.
grg added a commit that referenced this pull request Feb 14, 2024
Pass the ConstantFoldingPolicy instance to the ConstantFolding instance
in SpecializeAll. This was missed in #4406.
grg pushed a commit that referenced this pull request Apr 3, 2024
)

* Add constant folding policy with hook for the use case in #4402

* Add frotend policy to make unify access to frotend customizations

... and add a new one for optimizations

* Fix instantiation of frontend in TC

* Fix instatiation of frotend in tests

* MEV backport:
 - added optimizationLevel to CompilerOptions (port from future)
grg added a commit that referenced this pull request Apr 3, 2024
Pass the ConstantFoldingPolicy instance to the ConstantFolding instance
in SpecializeAll. This was missed in #4406.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants