-
Notifications
You must be signed in to change notification settings - Fork 453
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
base: main
Are you sure you want to change the base?
Conversation
8d17ce1
to
988e1b6
Compare
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]>
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]>
d13e3d7
to
22142e6
Compare
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]>
There was a problem hiding this 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.
/// @brief Get a candidate name for the instantiation. | ||
std::string name() const; | ||
|
||
bool operator<(const SpecSignature &other) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to double check: we're enforcing lexicographical order of SpecSignatures
in TypeSpecializationMap
. Maybe we should put signatures with less arguments first? Not sure if this would matter much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing depends on the order, except that it has to be a total order for the sake of std::map
. The insertion order is always driven by the "dependencies" that have to be defined first, and only when they are met for multiple elements then the order of the TypeSpecializationMap::map
decides -- and even then I've left this to be ordered_map
to make the order invariant on renaming of types. There is probably a better way to do this, but maybe that can be investigated in #5139.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. My point is that with cstring:operator<
we order everything lexicographically. But maybe we do not need to perform that amount of string comparisons.
Signed-off-by: Vladimír Štill <[email protected]>
@vlstill Looks like generic-struct.p4 is failing |
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
The reason is that the specialization of
S2
is inserted after specialization ofS1
which refers to it.