[#4656] Explicitly delay constant folding of only action enum IR::SwitchCase label expressions, instead of delaying constant folding of all IR::Mux expressions

[kfcripps]

This is one possible fix for #4656.

For example, one possible alternative might be to revert #3633 and add a check for action enum SwitchStatement labels, similar to the one in the TypeInference pass, to ValidateParsedProgram.

[kfcripps]

@jphurl @oleg-ran-amd I cannot request your review, but feel free to review this too.

[jafingerhut]

I am only reviewing the new test program and its expected outputs, not the C++ code changes inside the compiler. That part of this PR looks good to me.

[kfcripps]

The failing error tests need to be debugged..

[kfcripps]

It looks like #3699 was not ever fully fixed. This PR seems to also fully fix #3699.

[fruffy]

I wonder how expensive this search could be for deeply nested expressions...
In the worst case it will traverse to the root for each mux expression.

Maybe #3622 should be valid code instead?

[kfcripps]

I thought that #3633 was added because:

1. P4 spec says that action enum switch labels can only be action names, and Do not constant fold mux before it has been typechecked #3633 prevents them from being mux expressions that evaluate to action names at compile-time
2. P4 spec says that mux cannot act on action decls at all

I have not looked at the spec to verify (1) or (2). I am just going by what was stated in #3622 (comment).

I am fine with reverting #3633 if you don't think we actually need the error message added by it.

[fruffy]

Yeah I have been thinking about changing the specification. I wonder whether there are any downsides in having compile-time-known ternary operator support for labels.

I just did some quick performance measurements and this change does not cause that significant of a degradation for me, which is good.

Would be great to have an approach that does not require searching for the context for every Mux, but I can not think of one right now.

[kfcripps]

Maybe we should just revert #3633 for now and reopen #3622 until we decide whether to change the spec or not? I also cannot think of any downside in having compile-time-known ternary operator support for labels.

[asl]

I wonder how expensive this search could be for deeply nested expressions...
In the worst case it will traverse to the root for each mux expression.

I really do not think it is a problem. There are lots of much more severe overheads in other places, so this is just noise. We can certainly think of speeding-up findContext in general :)

[fruffy]

Well, it can have some impact for analysis tools that use constant folding on complex expressions. In the case of the tool I am using, it looks like it causes a ~6% slowdown on larger programs. But I understand that this is a non-standard use of the pass.

My concern is that this can be a death-by-a-thousand-paper-cuts type of situation. The transform pass can definitely be improved to be faster etc but we are also introducing some inefficient lookups here.

[asl]

My concern is that this can be a death-by-a-thousand-paper-cuts type of situation.

And we are already here actually :)

The transform pass can definitely be improved to be faster etc but we are also some inefficient lookups here.

+1

[kfcripps]

@fruffy Hopefully the recent changes will eliminate your concerns about the performance.

[fruffy]

This is a good solution, thanks!

[asl]

LGTM, thanks!

[ChrisDodd]

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.

[fruffy]

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...

[kfcripps]

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.

[kfcripps]

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..

[ChrisDodd]

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.

[kfcripps]

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.

[kfcripps]

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.

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:

1. Fix Constant folding of bit width expressions is no longer complete #4656 by merging [#4656] Revert PR #3633 so that mux expressions will be constant folded #4660, and reopen true ? action1 : action2 is accepted as a label for switch #3622 for the purpose of discussing whether the original error added by Do not constant fold mux before it has been typechecked #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 Constant folding of bit width expressions is no longer complete #4656. IMO having the error added by Do not constant fold mux before it has been typechecked #3633 is not as important as having constant folding of IR::Mux expressions, so Constant folding of bit width expressions is no longer complete #4656 has priority over true ? action1 : action2 is accepted as a label for switch #3622 and true ? action1 : action2 is accepted as a label for switch #3622 can be resolved later.

2. I can try using some custom function instead of containsActionEnum() or isActionRun() (and using it in an IR::SwitchStatement preorder 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;
}

3. 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.

[vlstill]

This looks like a reasonable compromise to me. I only have some tweak suggestions for the preorder(IR::SwitchCase *c).

[fruffy]

Just added some optional nits, approving from my side. Thanks for finding a good solution here!

[fruffy]

How about using the preorder function for IR::SwitchStatement then? Otherwise, I would just add a comment explaining why the static_cast was used.

[kfcripps]

@fruffy Is there a known issue with the CI? Looks like it is stuck

[fruffy]

@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.

[kfcripps]

merging soon if no other concerns

[fruffy]

Approving from my side.