p4c
p4c copied to clipboard
p4lang
[#4656] Explicitly delay constant folding of only action enum `IR::SwitchCase` `label` expressions, instead of delaying constant folding of all `IR::Mux` expressions
This is one possible fix for #4656.
For example, one possible alternative might be to revert https://github.com/p4lang/p4c/pull/3633 and add a check for action enum SwitchStatement labels, similar to the one in the TypeInference pass, to ValidateParsedProgram.
@jphurl @oleg-ran-amd I cannot request your review, but feel free to review this too.
It looks like #3699 was not ever fully fixed. This PR seems to also fully fix #3699.
What about case statements that contain non-mux constant expressions? Previously these would be folded. Perhaps only folding them after typechecking is fine? I don't see any testcases that excercise this currently.
What about case statements that contain non-mux constant expressions? Previously these would be folded. Perhaps only folding them after typechecking is fine?
Agh right. We may have switch statements that are not switching on tables but on normal expressions. But this should be okay since the expressions are folded after type-checking has completed? A little fragile though...
What about case statements that contain non-mux constant expressions? Previously these would be folded. Perhaps only folding them after typechecking is fine?
The purpose of #3633 was to make sure that only action names are used in labels of action enum switch statement cases. It did not actually do that. It only made sure that mux expressions were not used in labels of action enum switch statement cases. Now, any non-action-name expression in switch statement case labels will not be folded and can be inspected in the first type inference pass.
The side-effect is that regular switch statement case labels are no longer folded until after the first type inference pass, which I think is ok.
The side-effect is that regular switch statement case labels are no longer folded until after the first type inference pass, which I think is ok.
Unfortunately, I don't think that a switch statement's type is known until after the first type inference pass, so it cannot be checked in the first constant folding pass..
The side-effect is that regular switch statement case labels are no longer folded until after the first type inference pass, which I think is ok.
My concern is if there's some code that would flag the case label expression as not being a constant if it hasn't been folded yet. Maybe add a testcase like that to see what happens. We have a number of tests with switch(field) or switch(const) with some bit<n> type, but all seem to have simple constant literals as case labels.
The side-effect is that regular switch statement case labels are no longer folded until after the first type inference pass, which I think is ok.
Unfortunately, I don't think the type of switch statement that a switch statement is is known until after the first type inference pass, so it cannot be checked in the first constant folding pass..
Actually, let me see if I can make use of TypeInference::containsActionEnum() in DoConstantFolding somehow.
The side-effect is that regular switch statement case labels are no longer folded until after the first type inference pass, which I think is ok.
Unfortunately, I don't think the type of switch statement that a switch statement is is known until after the first type inference pass, so it cannot be checked in the first constant folding pass..
Actually, let me see if I can make use of
TypeInference::containsActionEnum()inDoConstantFoldingsomehow.
Yeah, checking whether a switch statement's expression is an action_run expression will be difficult in the first constant folding pass, as types are not yet known there. Neither TableApplySolver::isActionRun() nor TypeInference::containsActionEnum() will work there.
Unfortunately, having compile-time constant expressions in regular switch case labels will now result in an error on this branch, which is not good.
So the options are:
-
Fix #4656 by merging #4660, and reopen #3622 for the purpose of discussing whether the original error added by #3633 is actually necessary. If it is decided that it is necessary, then find a new solution to adding the error message that doesn't cause a functional problem such as the one described in #4656. IMO having the error added by #3633 is not as important as having constant folding of
IR::Muxexpressions, so #4656 has priority over #3622 and #3622 can be resolved later. -
I can try using some custom function instead of
containsActionEnum()orisActionRun()(and using it in anIR::SwitchStatementpreorder function), but it will not be able to properly detect action_run expressions in all cases without types being known. For example, something like this:
bool couldBeActionRun(const IR::Expression *e) {
const auto* mem = e->to<IR::Member>();
if (!mem) return false;
if (mem->member.name != IR::Type_Table::action_run) return false;
const auto* mce = mem->expr->to<IR::MethodCallExpression>();
if (!mce) return false;
if (mce->method->name.name != IR::IApply::applyMethodName) return false;
return true;
}
- Something else
My preference is (1), or (3) if somebody has a more robust solution that they want to suggest.
Also, I have added a test that shows the problem brought up by @ChrisDodd.
I suggest we go with https://github.com/p4lang/p4c/pull/4660 and fix https://github.com/p4lang/p4c/issues/3622 differently. I suggest we add a specific validation for that that runs even before constant folder. It cant be in ValidateParsedProgram as we would need a ref map, but I think this can be done without a type map.
- find all switch statements, if its "condition" is
action_runmember, check that it is in anapplycall on a table -- to resolve that we don't need a type map, just ref map - if so, check that all the labels are names of actions mentioned in the action list of the table
I don't think we should be bending the constant folder to special-case this. As I suggested in https://github.com/p4lang/p4-spec/issues/1281, allowing this mux in label would, at least in my opinion, require extending the type-checking rules. Otherwise we are very quickly getting into territory where the program actually don't make much sense from the type-system point.
@vlstill I forgot that we can use refMap here to get the table's decl. Thanks!
I cannot spend the time moving the action_run SwitchStatement label error message somewhere before the first constant folding pass (as @vlstill suggested above), but I have added extra logic to the DoConstantFolding preorder function that uses refMap to explicitly check whether the SwitchCase's parent SwitchStatement has an action_run expression.
@fruffy @asl @ChrisDodd @vlstill If you see any problems with this, we can consider merging #4660 and reopening #3622 instead. Otherwise, please re-approve.
@fruffy Is there a known issue with the CI? Looks like it is stuck
No, nothing known. This seems like an issue on the Github side? It could also just be that we are currently consuming to many resources and are rate-limited.