Skip to content
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

The Evaluator pass is not able to properly complex instantiation arguments #5149

Open
vlstill opened this issue Feb 25, 2025 · 4 comments
Open
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)

Comments

@vlstill
Copy link
Contributor

vlstill commented Feb 25, 2025

The evaluator pass, which is running in frontend and in many other places in compilation, is not able to properly handle complex arguments in instantiations. Namely the cast operation does not work, and there is no corresponding representation for cast in P4::IR::CompileTimeValue. However, if cast is allowed, we can easily end up with having to replicate basically all expressions in the CompileTimeValue -- there can be compile time struct expressions, casts, struct field accesses, additions, etc (as compile time values can be arbitrary). I think this requires a different approach. Maybe we can get rid of CompileTimeValue completely and just enforce that the instantiation parameters can be constant evaluated in the result of evaluator -- I don't think it makes sense to replicate (almost) the entire expression tree in constant versions (for example, LLVM does this and it is a major pain).

@vlstill vlstill added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 25, 2025
@asl
Copy link
Contributor

asl commented Feb 25, 2025

I think instantiations should be explicitly resolved very early – somewhere close to generic types specializations. Constructor parameters effectively make "stateless" objects (per spec) something with internal state.

E.g. this is a valid code:

parser p(in int<10> sinit, out bool matches)(bool ctorParam) {
    int<10> s  = ctorParam ? sinit : 0;

    state start {
        s = 1;
        transition next;
    }

    state next {
        s = 2;
        matches = true;
        transition accept;
    }
}

While per spec ctorParam is a compile-time constant, its value is only known at instantiation time. This creates unnecessary complications as one needs to treat ctorParam as "constant with unknown value" while analyzing p. This also contradicts with what is written in spec:

Parsers and control blocks types are similar to function types: they describe the signature of parsers and control blocks.

So, it seems that in reality constructor parameters should be treated as "template arguments" and instantiated as early as possible.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 25, 2025

@asl, your point is a bit different than I had originally in mind. Sorry I did not provide the example right away.

Here is a snippet (full test that fails in p4test: evaluator.p4.txt)

    Register<ValueType, bit<4>>(8, (ValueType){
            c = 42,
            a = 1,
            b = -1,
            nested = (Nested){a = (TN16)4, b = 16}
        }) r0;
$ ./build/p4c/p4test evaluator.p4
evaluator.p4(26): [--Werror=invalid] error: { c = 8w42, a = 1w1, b = -1s1, nested = { a = (TN16)16w4, b = 16w16 } }: Cannot evaluate to a compile-time constant
...eType, bit<4>>(8, (ValueType){c = 42, a = 1, b = -1, nested = (Nested){a = (TN16)4, b = 16}}) r0;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Namely the problem is the (TN16)4, which cannot be represented, even this fails:

Register<TN16, bit<4>>(8, (TN16)4) r0;
$ ./build/p4c/p4test evaluator.p4
evaluator.p4(26): [--Werror=invalid] error: (TN16)16w4: Cannot evaluate to a compile-time constant
    @hidden Register<TN16, bit<4>>(8, (TN16)4) r0;
                                      ^^^^^^^

The problem is that EvaluatorPass tries to build the constant representation (using CompileTimeValue) which is different from Expression but CompileTimeValue is not expressive enough. I think this is fundamentally wrong to use a different IR subset for this. And EvaluatorPass runs already in frontend, before newtypes are removed.

This basically prevents any backend/target from using such expressions.

On the other hand, what @asl describes might depend on the target and the kind of object being instantiated (i.e. it might make sense for instances of control and parser, but not for extern instances). We should probably split that discussion.

@asl
Copy link
Contributor

asl commented Feb 25, 2025

This basically prevents any backend/target from using such expressions.

Oh, you are right. It might appear as different issue, but I think it is still the same generic question: what is considered as a compile-time constant, when and how they should be evaluated. The spec provides guidance for local compile-time values vs compile-time values, but this is from the language spec perspective, the compiler should implement these somewhere somehow.

I agree that there should be no "whole constant expression hierarchy" and some canonical representation of the code (with constants folded / evaluated in proper places) should be made.

@asl
Copy link
Contributor

asl commented Feb 27, 2025

@vlstill actually, if we'd modify my example above we'll end with complex constant expression as well:

parser p(in int<10> sinit, out bool matches)(bool ctorParam) {
    const int<10> c = ctorval ? 10s10: 10s42;
    int<10> s  = sinit;

    state start {
        s = 1;
        transition next;
    }

    state next {
        s = 2;
        matches = true;
        transition accept;
    }
}

Here c will be Declaration_Constant, but its initializer will be Mux with pretty weird condition :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

No branches or pull requests

2 participants