[#4661] Do not unconditionally mark extern method calls as compile-time constants.

[kfcripps]

constArgs is used to indicate whether all of an extern function call's arguments are constant, in which case the call is marked as a compile-time constant. The constArgs calculation doesn't consider extern functions that have 0 arguments. see below discussion

Fixes #4661.

[ChrisDodd]

This is kind-of ugly with constArgs being used for two different cases -- extern methods that are "factory" methods to create an extern instance (which are allowed iff the arguments are all constants, and should treat no arguments as arguments as allConst), and to mark the resulting expression as a constant (which should only be the case if the method is @pure. So the correct fix might actually be to add a check in at line 3844 to only do this latter thing if the function has an @pure annotation.

[ChrisDodd]

I think the correct fix might be to add a check in at line 3844 to only mark the expression as constant if the extern method has an @pure annotation (and all the args are constants).

if (auto ef = mi->is<ExternFunction>()) {
    if (constArgs && ef->method->getAnnotation(IR::Annotation::pureAnnotation)) {

[kfcripps]

@ChrisDodd I don't think constArgs is at all related to the @pure annotation. constArgs was added ~6 years ago in #1187, and @pure was later added ~4 years ago in #2302.

[kfcripps]

But I don't really understand why we would want to mark extern method calls as compile-time constants when all args are const (or when there exist no args). Isn't it up to a backend to decide whether a given extern method can be evaluated at compile-time or not?

[ChrisDodd]

But I don't really understand why we would want to mark extern method calls as compile-time constants when all args are const (or when there exist no args). Isn't it up to a backend to decide whether a given extern method can be evaluated at compile-time or not?

Yes, precisely -- and the backend should mark the extern as @pure if it is pure (depends solely on the arguments). So when all the arguments (if any) are constant AND the function is @pure, it should be considered a compile-time constant. The problem is that when the code for checking the arguments and deciding when an expression (call) is a compile-time constant was added, the @pure annotation did not yet exist, so the code just assumed all extern functions were pure.

The other place this constArgs is used is around line 3793 -- that (experimental) code allows calls to an extern function that returns an extern instance with all const args without flagging an error. This is for a "factory" extern function that creates an extern instance (much like a ctor for an extern type) at compile time -- normally we require that all extern instances be statically resolvable at compile time (no dynamic allocation on the data plane!). There should perhaps be another annotation (@factory?) for such an extern method, but currently we don't have such.

In both places, a function of no arguments should be considered to be a function with all arguments constant (thus the constArg = true; init before the loop over the arguments.

[kfcripps]

@ChrisDodd The code on line 3844 is also needed for factory methods (and static_assert - test issue3531.p4 will fail without marking static_assert calls as compile-time constants). So, we probably need to introduce @factory annotation to handle this properly (note I have added @pure annotations to factory1.p4 and factory2.p4 tests for now). If you agree I can add the annotation and also check for it on that line, in addition to the checks for @pure and static_asset.

[ChrisDodd]

Yes, using @pure on the factory methods is an abuse, but probably ok for now. What we really want is something like C++ constexpr, but I don't think I really want to go that way either. static_assert is another crufty bit -- in C++ it is a special form, and not "function" you call (though it looks syntactically like a function).

I now can't recall why were adding these experimental factory-style functions -- someone probably wanted them, but I'm not sure why. We could hack in a flag when the return type is an extern type, but that isn't nice either. These testcases never made it into any "real" backend as far as I can see.

[kfcripps]

@fruffy @ChrisDodd Do you know what is wrong with my DCO remediation commit?

[fruffy]

@fruffy @ChrisDodd Do you know what is wrong with my DCO remediation commit?

Hmm maybe because there is Kyle Cripps and kfcripps as signers? I do not thnk there is a way to configure DCO to ignore this.

Btw DCO isn't mandatory yet until kinks like that are sorted out.

[kfcripps]

Hmm maybe because there is Kyle Cripps and kfcripps as signers? I do not thnk there is a way to configure DCO to ignore this.

but in #4762, I also signed off as "Kyle Cripps" and it worked :(

[vlstill]

Yes, precisely -- and the backend should mark the extern as @pure if it is pure (depends solely on the arguments). So when all the arguments (if any) are constant AND the function is @pure, it should be considered a compile-time constant.

I don't think it is safe to assume that a @pure extern invocation that has compile-time constant arguments it itself compile-time constant. @pure only says that certain optimizations can be done (elimination of calls with unused results, combining of calls). But the backend may still implement by invoking something that is not part of P4 and it might not be able (or willing) to compute it at compile time. Example could be some sort of hash that is implemented by a fixed-function block in the design -- the compiler could optimize it, but not evaluate. So I think @pure should not be abused for this.

[kfcripps]

@vlstill Are you saying we should replace @pure with @factory in tests factory1.p4 and factory2.p4, or just remove @pure and update the tests? Those tests result in an error message without the extern function call being marked as a compile-time constant.

[fruffy]

Hmm maybe because there is Kyle Cripps and kfcripps as signers? I do not thnk there is a way to configure DCO to ignore this.

but in #4762, I also signed off as "Kyle Cripps" and it worked :(

Commit sha: d13f6fd, Author: Kyle Cripps, Committer: kfcripps; The sign-off is missing.
Commit sha: 3d32b54, Author: Kyle Cripps, Committer: Kyle Cripps; The sign-off is missing.
Commit sha: 1fe771c, Author: Kyle Cripps, Committer: Kyle Cripps; The sign-off is missing.
Commit sha: 952cbaa, Author: Kyle Cripps, Committer: Kyle Cripps; The sign-off is missing.

This is the actual implementation:
https://github.com/dcoapp/app/blob/main/lib/dco.js#L140

It might be a mismatch between commit author and signoff. Unclear to me. It might be easier just to amend and rebase...

[vlstill]

@vlstill Are you saying we should replace @pure with @factory in tests factory1.p4 and factory2.p4, or just remove @pure and update the tests? Those tests result in an error message without the extern function call being marked as a compile-time constant.

Basically what I am saying is that if we want this behaviour (compiler-evaluated call to extern with constant args), we should have a specific annotation for that. I'm not sure if the "factory" is the only use case we have for these.

On a second thought, these two things are not even one extension of another. It is possible that a compiler will be able to constant-evaluate an extern that is not pure! For example, one can imagine a seqCount() extern for which each (syntactic) invocation will be replaced by number of previous (syntactic) invocations. Definitely not pure, but compiler-evaluated (also probably not useful, but my point is that @pure should not be used for this).

What do you think, @ChrisDodd?

[ChrisDodd]

On a second thought, these two things are not even one extension of another. It is possible that a compiler will be able to constant-evaluate an extern that is not pure! For example, one can imagine a seqCount() extern for which each (syntactic) invocation will be replaced by number of previous (syntactic) invocations. Definitely not pure, but compiler-evaluated (also probably not useful, but my point is that @pure should not be used for this).

What do you think, @ChrisDodd?

Yes, they're not the same. @pure means that the compiler has all the information it needs at compile time to evaluate it, not that it must (and that it can eliminate it if unused, or PRE combine multiple calls). With factory methods, they're not pure (can't PRE combine them), and if the compiler doesn't evaluate them at compile time we end up with dynamic behavior which is like not supportable in many data planes. Your proposed seqCount() is much like this. I don't really like @factory as an annotation, but I'm unable to come up with anything better. The previous code was basing it on the return type -- if the return type is an extern instance then it must be evaluable at compile time or you end up with dynamic data-plane behavior.

[vlstill]

I don't really like @factory as an annotation, but I'm unable to come up with anything better.

@ChrisDodd, we could borrow consteval from C++20 and make a @consteval annotation. I think that what we want in these cases is probably exactly the same as the C++20 version, and the C++ keyword is quite well named for once. Therefore, reusing it would not be confusing.

https://en.cppreference.com/w/cpp/language/consteval

• consteval - specifies that a function is an immediate function, that is, every call to the function must produce a compile-time constant

Would this be something to propose to the spec first?

[kfcripps]

@vlstill @ChrisDodd For now, I have modified the check to mark extern function call results as compile-time constants only if:

• It is a static_assert() call, or
• The result is of type extern

This is an improvement from the main branch behavior, which always marks the results of extern function calls as compile-time constants. It is also an improvement from the previous behavior of this branch, which always marked the results of pure extern function calls as compile-time constants.

I believe adding a new annotation goes beyond the fix for #4726 and is out of scope of my intended changes.

[kfcripps]

@fruffy Is container-image / build job broken on main branch? I see it also failing in #4791

[fruffy]

@fruffy Is container-image / build job broken on main branch? I see it also failing in #4791

These are spurious failures from the debian repositories, annoying but tricky to fix.

[fruffy]

I set DCO to pass, it's not yet required anyway.

[kfcripps]

Thanks @fruffy