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

allow disabling of subtraction transform #4633

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Conversation

grg
Copy link
Contributor

@grg grg commented Apr 19, 2024

Add mechanism to allow the the transform that changes a - const -> a + (-const) to be disabled.

This change adds a new function to the FrontEndPolicy object to control this optimization. We may eventually want to change to some other mechanism (e.g., breaking out some of the optimizations that aren't universally useful), but for now this provides the ability to turn the optimization off.

Add mechanism to allow the the tranform that changes
`a - const` -> `a + (-const)` to be disabled
@grg grg force-pushed the grgibb/transform_disable branch from aaab525 to 3734460 Compare April 19, 2024 20:17
@grg grg marked this pull request as ready for review April 20, 2024 05:10
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

I think GCC/CLang-style feature flags might be preferable over adding a policy to each compiler pass.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Apr 20, 2024
@vlstill
Copy link
Contributor

vlstill commented Apr 22, 2024

@grg, out of curiosity can you share motivation for disabling this rewriting?

These changes are increasingly showing that the current frontend/midend split is not really working. Indeed even which frontend optiomizations disabled, it does actually a lot of optimizations and not really necessary rewriting (constant folding, parser-if elimination, control flow simplications). One path forward I could see would be to start definting a "minimal frontend" that would be basically responsible of checking validity of the program, including type checking but would not very little rewriting. It might be tricky to say where the limits should be (inserting casts explicitly is probably OK, expanding the ... default values likely too, but constant folding or removal of useless casts is not OK I'd say). SideEffectsOrdering is probably also outside of frontend. Then we could define the exiting frontend to start with these passes and the targets that want would switch to the new frontend and move all the optimizations they want to midend.

What could get tricky is that some midend passes may have implict preconditions that the current frontend (some pass from it) had already executed.

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

How does this affect constant folding, is the pass able to handle subtract of constants?

Otherwise the code looks good to me ✅.

@grg
Copy link
Contributor Author

grg commented Apr 22, 2024

@vlstill A simple array reference of this form (where depth is a 2 bit field):

hdrs.ipv4[meta.common.depth-1]

is translated to:

hdrs.ipv4[meta.common.depth + 2w3]

The Intel IPU compiler reports an error after this translation is applied. Without knowing the history, my guess is that the original code author wanted to prevent access beyond the current depth by disallowing any addition to the depth. After the frontend has executed and applied this transformation, we don't know whether the P4 author wrote depth - 1 or depth + 3.

I agree that defining a new minimal front-end is a good way to proceed. Some existing passes will neatly fit either in-or-out of the minimal front-end, but I suspect that there will be some passes that need splitting.

@grg grg added this pull request to the merge queue Apr 22, 2024
Merged via the queue into main with commit e304163 Apr 22, 2024
17 checks passed
@grg grg deleted the grgibb/transform_disable branch April 22, 2024 16:07
@ChrisDodd
Copy link
Contributor

These changes are increasingly showing that the current frontend/midend split is not really working. Indeed even which frontend optiomizations disabled, it does actually a lot of optimizations and not really necessary rewriting (constant folding, parser-if elimination, control flow simplications).

The genesis of this (doing "too much" in the frontend) is allowing bit<expr> where expr is any expression that can be reduced to a constant. We need that in order to do typechecking properly (which we want in the frontend), so we end up needing inlining (in case the expression calls some function), which in turn brings in lots of other requirements as well.

One possibility is that we set things up to do these passes that should not be in the frontend only in limited contexts (like type arguments) when running in the frontend, so as to allow proper typechecking, without doing everything that any target might not like. Then targets could run them (or not) in the midend as desired.

What could get tricky is that some midend passes may have implict preconditions that the current frontend (some pass from it) had already executed.

This is already a general problem -- many passes depend on some other pass having been run first, and it is not well documented. Passes that have preconditions should at least document those preconditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants