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

frontend: Fix SpecializeGenericTypes #5133

Merged
merged 18 commits into from
Mar 17, 2025

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Feb 19, 2025

A significant rewrite of SpecializeGenericTypes. Namely it fixes problems occurring in more complex specializations where a specialized type might depend on another specialized type and their order of insertion can be incorrect. Instead the types are now inspected to check for their dependencies and inserted just after all of the dependencies. I've also change the naming scheme of the specializations so that simple once are more readable (although for complex once the types quickly becomes ugly).

A caveat is that many passes seem to use very similar, but slightly different patterns and these passes still contain bugs (e.g. EliminateTuples. SpecializeGenericFunctions). Ideally, we would want to factor out the common parts and implement only the specific once, but I don't have time for that (at least not now). Namely at least the insertion part should be factorable for any pass that inserts global objects that can have dependencies.

Without the changes, the added test fails with

1217: Error compiling
1217: In file: ./p4c/frontends/p4/typeChecking/typeChecker.cpp:144
1217: type-spec-nested.p4(3): Could not find type of <Type_Struct>(3478) S2_0/2 struct S2_0 {
1217:   int<6> x;
1217:   int<6> y; }
1217: struct S2<T> {
1217:        ^^

The reason is that the specialization of S2 is inserted after specialization of S1 which refers to it.

fixes #4835

@vlstill vlstill added bug This behavior is unintended and should be fixed. core Topics concerning the core segments of the compiler (frontend, midend, parser) labels Feb 19, 2025
@vlstill vlstill self-assigned this Feb 19, 2025
@vlstill vlstill force-pushed the fix-nested-generic-spec branch from 8d17ce1 to 988e1b6 Compare February 19, 2025 19:45
@vlstill vlstill marked this pull request as draft February 20, 2025 06:59
@vlstill vlstill changed the title frontend: Fix and simplify specializeGenericTypes frontend: Fix specializeGenericTypes Feb 21, 2025
@vlstill vlstill force-pushed the fix-nested-generic-spec branch 2 times, most recently from d13e3d7 to 22142e6 Compare February 21, 2025 19:19
@vlstill
Copy link
Contributor Author

vlstill commented Feb 21, 2025

@fruffy or anyone familiar with Bazel, any idea what is behind the bazel failure (e.g. here)? I can see it did not find absl/strings/str_replace.h, but I don't know how to find it as there is no @com_google_absl//absl/strings:str_replace.

@vlstill vlstill changed the title frontend: Fix specializeGenericTypes frontend: Fix SpecializeGenericTypes Feb 21, 2025
@vlstill vlstill marked this pull request as ready for review February 21, 2025 19:30
@vlstill vlstill requested review from ChrisDodd and fruffy February 24, 2025 08:50
Copy link
Contributor

@asl asl left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. See some minor notes and questions.

@asl
Copy link
Contributor

asl commented Feb 27, 2025

@vlstill Looks like generic-struct.p4 is failing

@vlstill
Copy link
Contributor Author

vlstill commented Mar 3, 2025

@vlstill Looks like generic-struct.p4 is failing

Yep, there was still some dependency on processing order, it should now be fixed hopefully.

@vlstill vlstill requested a review from asl March 3, 2025 19:54
@vlstill vlstill force-pushed the fix-nested-generic-spec branch from 1bf8c88 to 3cdff52 Compare March 10, 2025 07:19
vlstill added 7 commits March 11, 2025 06:35
Always place the specialized structs *right before* the original.

Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
vlstill and others added 9 commits March 11, 2025 06:35
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Co-authored-by: Anton Korobeynikov <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Co-authored-by: Anton Korobeynikov <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
Signed-off-by: Vladimír Štill <[email protected]>
@vlstill vlstill requested a review from antoninbas March 11, 2025 08:00
@antoninbas antoninbas requested review from jafingerhut and removed request for antoninbas March 11, 2025 08:16
@vlstill
Copy link
Contributor Author

vlstill commented Mar 11, 2025

The ptf-ebpf failure seems unrelated, likely a missing dependency (I don't see why though). But this is not a required check, so we should be able to merge anyway once this is approved.

It passed after rerun.

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.

Not very familiar with that pass so it might take me a bit to give a proper review. Other might be better qualified for that.

@@ -0,0 +1,50 @@
#include <core.p4>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhat of a nonstandard issue name for the file? Although we do not really have a consistent naming scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sometimes I forget which conventions differ between projects, renamed to be more consistent :-)

Copy link
Contributor

@asl asl left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@vlstill vlstill enabled auto-merge March 17, 2025 12:23
Signed-off-by: Vladimír Štill <[email protected]>
@vlstill vlstill added this pull request to the merge queue Mar 17, 2025
Merged via the queue into p4lang:main with commit 99f2441 Mar 17, 2025
20 checks passed
@vlstill vlstill deleted the fix-nested-generic-spec branch March 17, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This behavior is unintended and should be fixed. 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.

Compiler Bug: Could not find type of <Type_Header> ... on specialized generic struct type
3 participants