Dyno: Type-convert select-statements #27095
Conversation
Signed-off-by: Ben Harshbarger <[email protected]>
Signed-off-by: Ben Harshbarger <[email protected]>
There was a problem hiding this comment.
This looks awesome, my only worry is that we should probably avoid constructing temps without types {} where possible (for now), e.g., for the storeInTempIfNeeded calls, as there are some situations where calling ->qualType() or ->typeInfo() can trigger the old resolver.
| Symbol* t = nullptr; | ||
|
|
||
| if (qt.isUnknown()) { | ||
| t = makeNewTemp(e->qualType().getQual(), e->typeInfo()); |
There was a problem hiding this comment.
The problem with this is that there are at least some paths through ->qualType() which seem to invoke the old resolver which is why I decided to only allow temps created with the chpl::types::QualifiedType. If we could figure out some way to translate without invoking the old resolver this could be done.
E.g., I know there are some primitives where the types are determined via the old resolver. I have not done due diligence but there could be more.
Maybe we hold off on adding this sort of thing if we can and if we do make sure via sanity checks/crashes that it can't actually invoke the old resolver.
| @@ -4050,7 +4068,8 @@ void TConverter::ActualConverter::convertActual(const FormalActual& fa) { | |||
| INT_ASSERT(astActual); | |||
|
|
|||
| // Convert the actual and leave. | |||
| std::get<Expr*>(slot) = tc_->convertExpr(astActual, rv_); | |||
| auto actualExpr = tc_->convertExpr(astActual, rv_); | |||
| std::get<Expr*>(slot) = tc_->storeInTempIfNeeded(actualExpr, {}); | |||
There was a problem hiding this comment.
Do we always need to have the actuals be temps?
| @@ -5071,6 +5090,157 @@ bool TConverter::enter(const ExternBlock* node, RV& rv) { | |||
|
|
|||
| void TConverter::exit(const ExternBlock* node, RV& rv) {} | |||
|
|
|||
| static SymExpr* makeCaseCond(TConverter& tc, TConverter::RV& rv, | |||
There was a problem hiding this comment.
IMO we should normalize either TConverter* or TConverter& for static helper functions so that they're consistent, I don't really have a preference here but usually just do TConverter* cause then you can pass in this. Thoughts?
| @@ -4050,7 +4068,8 @@ void TConverter::ActualConverter::convertActual(const FormalActual& fa) { | |||
| INT_ASSERT(astActual); | |||
|
|
|||
| // Convert the actual and leave. | |||
| std::get<Expr*>(slot) = tc_->convertExpr(astActual, rv_); | |||
| auto actualExpr = tc_->convertExpr(astActual, rv_); | |||
| std::get<Expr*>(slot) = tc_->storeInTempIfNeeded(actualExpr, {}); | |||
There was a problem hiding this comment.
See the TODO, I think it would be good to have the callsites supply the types unless we're sure we're not going to accidentally invoke the old resolver.
So just get the type from convertExpr for now?
| // it's a bit confusing for the case-expression's type in 'rv' to always be | ||
| // a bool. | ||
| Expr* caseExpr = tc.convertExpr(cs, rv); | ||
| caseExpr = tc.storeInTempIfNeeded(caseExpr, {}); |
There was a problem hiding this comment.
Can we get the type from convertExpr and pass that to storeInTempIfNeeded here?
| @@ -812,6 +812,8 @@ static void handleForallGoto(ForallStmt* forall, GotoStmt* gs) { | |||
|
|
|||
| static void resolveGotoLabels() { | |||
| forv_Vec(GotoStmt, gs, gGotoStmts) { | |||
| if (gs->parentSymbol->hasFlag(FLAG_RESOLVED_EARLY)) continue; | |||
| auto cmp = action->fn(); | ||
|
|
||
| // TODO: create a wrapper for this kind of thing | ||
| const ResolvedFunction* rf = nullptr; |
There was a problem hiding this comment.
I agree, maybe the wrapper can return the converted FnSymbol* or something as well as whether or not it was param elided, we might have enough uses of this to get a sense of the pattern here.
| // equality comparisons are not evaluated unless the preceding cases do | ||
| // not match. | ||
|
|
||
| VarSymbol* agg = new VarSymbol("_case_cond_agg", dtBool); |
There was a problem hiding this comment.
Can we use a unnamed temp here instead? I ask because just the other day I ran into a bug where I decided to name a temp chpl_error, and that actually caused the backend to be unable to locate the runtime function chpl_error. I feel like just using unnamed temps wherever possible might eliminate any chance of name conflicts.
| when y, 1, 2 do println(1); // mixture of param and value | ||
| when 6 do println(42); // param only | ||
| when paramValue() do println(777); // param returned via procedure | ||
| otherwise do println(999); // otherwise |
There was a problem hiding this comment.
I love the combination of numbers lol.
This PR adds initial support for type-converting select-statements using resolution info from dyno. Like production, this PR translates select-statements into if-statements.
One difference with production concerns a when-statement with multiple cases, where one of those cases is param-true. Historically we have executed a comparison for all case-exprs up to the param-true case. In this PR, we no longer execute those comparisons when any case-expr is param-true.
Testing: