-
Notifications
You must be signed in to change notification settings - Fork 217
WIP: Add constexpr if #3268
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
base: main
Are you sure you want to change the base?
WIP: Add constexpr if #3268
Conversation
| // Later during the generation of TypeInfo, after resolving parametrics, | ||
| // and evaluating the test condition, we force the type of the selected | ||
| // if branch on the conditional node. | ||
| XLS_RETURN_IF_ERROR( |
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.
I think this would be more in-model if we would define a new type of internal type annotation, like ConstConditionalTypeAnnotation, and set an instance of that on the Conditional node in the const case. The ConstConditionalTypeAnnotation could have the const test Expr as a member, and a TVTA for each possible branch.
TypeAnnotationResolver would then perform the resolution logic that you currently have inside GenerateTypeInfo (in a manner similar to MemberTypeAnnotation, ElementTypeAnnotation, etc.).
Aside from that this all looks reasonable.
| table_.SetTypeVariable(node->consequent(), type_variable)); | ||
| XLS_RETURN_IF_ERROR( | ||
| table_.SetTypeVariable(ToAstNode(node->alternate()), type_variable)); | ||
| if (!node->IsConst()) { |
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.
Usually we prefer to use positive conditions in a C++ if statement, even if it leads to the less common branch being first (so flip the 2 branches here).
Signed-off-by: Robert Winkler <[email protected]>
d236527 to
43ce9af
Compare
Signed-off-by: Robert Winkler <[email protected]>
Signed-off-by: Robert Winkler <[email protected]>
Signed-off-by: Robert Winkler <[email protected]>
Signed-off-by: Robert Winkler <[email protected]>
Signed-off-by: Robert Winkler <[email protected]>
43ce9af to
7eaf714
Compare
This PR adds support for the constexpr
ifexpression.Information about the
constmodifier is stored in theConditionalclass and used later during type checking. For constexprif, separate type variables are assigned to each branch of the condition. At this stage, the result is not directly bound to them. During theTypeInfogeneration, the constexpr condition is evaluated, and one branch of the if is selected. Its type is unified, and the entire if is annotated with that type. The bytecode emitter and IR converter have been updated to inline the selected branch, skipping the unchosen one.Proper tests for this feature are still needed and will be added soon.
One issue not yet handled correctly in this PR is the conditional spawning of procs, for which an example has been added that shows the problem. Currently, all spawns affect the type, which should not happen as only the selected branch of constexpr
ifshould be considered. One possible approach is to enable constexpr evaluator in thePopulateInferenceTableVisitorand skip traversing the branch that was not taken. However, thePopulateInferenceTableVisitordoes not have access to the parametric environment, and the condition relying on the parametrics seems to be the most imprtant use-case for using constexprifwhen spawning procs.