extend type checking to mark extern function call that returns enum a… #4941

hanw · 2024-10-03T23:06:13Z

…s compile time constant

This is an regression fix related to #4726

In tofino backend, we have a use case that requires treating extern function returning enum as a compile-time constant. Example in hashext3.p4

grg · 2024-10-03T23:34:59Z

testdata/p4_16_samples/hashext3.p4

@@ -0,0 +1,52 @@
+/*
+Copyright 2018 Barefoot Networks, Inc.


Should we update this to Intel 2024?

kfcripps

I think you'd either need to mark your backend extern function as @pure, or introduce the @consteval annotation suggested by @vlstill. I don't think there is anything special about extern functions that return enum that guarantees that these types of functions will evaluate to compile-time constants.

…s compile time constant Signed-off-by: Wang, Han2 <[email protected]>

hanw · 2024-10-04T03:17:50Z

I added support for @pure since it's already in the compiler.

I like @vlstill 's proposal, we can add that after drafting a change to spec.

asl · 2024-10-04T03:32:32Z

ir/expression.cpp

@@ -16,6 +16,8 @@ limitations under the License.

 #include <ostream>

+#include <boost/functional/hash.hpp>


I made a separate PR for this line.

…led-time constant update reference output Signed-off-by: Wang, Han2 <[email protected]>

hanw · 2024-10-04T16:37:38Z

@kfcripps please review again. let me know if you are happy with the change.

ChrisDodd · 2024-10-04T21:59:31Z

frontends/p4/typeChecking/typeCheckExpr.cpp

-            if (constArgs && factoryOrStaticAssert) {
+            const bool hasPureAnnotation =
+                ef->method->getAnnotation(IR::Annotation::pureAnnotation);
+            if (constArgs && (factoryOrStaticAssert || hasPureAnnotation)) {


Seems a little awkward to have two bools for three conditions.

Signed-off-by: Wang, Han2 <[email protected]>

kfcripps · 2024-10-05T11:45:48Z

frontends/p4/typeChecking/typeCheckExpr.cpp

            const IR::Type *baseReturnType = returnType;
            if (const auto *sc = returnType->to<IR::Type_SpecializedCanonical>())
                baseReturnType = sc->baseType;
-            const bool factoryOrStaticAssert =
-                baseReturnType->is<IR::Type_Extern>() || ef->method->name == "static_assert";
-            if (constArgs && factoryOrStaticAssert) {
+            const bool factoryOrStaticAssertOrPureAnnot =
+                baseReturnType->is<IR::Type_Extern>() || ef->method->name == "static_assert" ||
+                ef->method->getAnnotation(IR::Annotation::pureAnnotation);
+            if (constArgs && factoryOrStaticAssertOrPureAnnot) {
                // factory extern function calls (those that return extern objects) with constant
                // args are compile-time constants.
-                // The result of a static_assert call is also a compile-time constant.
+                // The result of a static_assert call is also a compile-time constant
+                // Pure functions marked with @pure annotation is a compiled-time constant
                setCompileTimeConstant(expression);
                setCompileTimeConstant(getOriginal<IR::Expression>());
            }


I am no expert on @pure, but according to @ChrisDodd and @vlstill's previous comments (#4726 (comment), #4726 (comment), #4726 (comment)), a pure extern function that has const args is not obligated to evaluate to a compile-time constant. Excluding such extern functions here was intentional in #4726. @ChrisDodd Can you double-check that this makes sense?

If you look at the example in this PR, would @consteval make more sense to you?

Yes, some consteval keyword or annotation makes the most sense to me based on my understanding of the previous comments from @vlstill and @ChrisDodd. @vlstill @ChrisDodd What do you think?

I'm of mixed mind about this. @pure has a particular meaning (basically, the function depends only on its arguments and has no side effects), so any @pure function where all the arguments are constants is (logically) a constant. But the term "compile-time constant" has a particular meaning in the spec, so it certainly seems plausible to have non-@pure functions that are also compile-time-constants if all the arguments are compile-time-constants. In particular, extern functions that do have some sort of run-time side effect, but can otherwise be treated as compile time constants, and should NOT be dead-code eliminated if their result is not used (which is an allowed optimization for @pure functions).

For this specific case, are the functions in question ones that should not be subject to dead-code elimination if the value they return is unused? If so, they should not be @pure and we need to come up with something else. The example looks the the functions are returning a handle to a hash function that can later be applied to some other data (so dead-code eliminating it if it never applied to anything seem fine).

Yes, dead-code eliminating the function if not used is fine.

So I've been reading through the code, trying to figure out everywhere that this isCompileTimeConstant attribute of expressions stored in the typeMap is used and what it is used for.

For the most part, it is used for checking if things are "compile-time known values" as defined by the spec, and issuing errors for things that should be compile-time known and are not. Not sure how the difference between "local compile-time known" and "compile-time known" applies here (important difference in the spec; not sure how it is reflected in the p4c code). The spec doesn't really say if the result of an extern function can be compile-time known or not; IMO it makes sense to leave it up to the arch (since the arch defines what extern functions are and what they mean), so an annotation on the extern function declaration makes sense here.

However, it also appears to be used in cases like sideEffects where it is a short-cut for enabling optimizations -- a value that isCompileTimeConstant is true is assumed to not be alias with modifications or otherwise interact with things that have side effects, so can be reordered with respect to them. The @pure annotation makes perfect sense for this case.

Not clear to me if we should think about breaking up these two cases. We should definitely be more clear about what exactly is compile-time know vs local compile-time known vs known to be a constant for optimization purposes, but that is something that belongs in its own issue. For the purpose of this change, I'm fine with overloading @pure to manage this, or with defining a new annotation if that is what people think is better.

Got a OK from Chris. Will merge for now to unblock other tasks.

hanw · 2024-10-08T01:46:24Z

@fruffy is the mac build broken?

fruffy · 2024-10-08T01:53:09Z

Looks like it, some failure related to a homebrew boost installation...

fruffy · 2024-10-08T02:26:56Z

I have made the MacOS test non-required until this dependency failure is fixed.

@pure

#4941) * extend type checking to mark extern function call that returns enum as compile time constant Signed-off-by: Wang, Han2 <[email protected]> * use @pure to annotate extern function that should be treated as compiled-time constant update reference output Signed-off-by: Wang, Han2 <[email protected]> * address comment Signed-off-by: Wang, Han2 <[email protected]> --------- Signed-off-by: Wang, Han2 <[email protected]>

@pure

#4941) * extend type checking to mark extern function call that returns enum as compile time constant Signed-off-by: Wang, Han2 <[email protected]> * use @pure to annotate extern function that should be treated as compiled-time constant update reference output Signed-off-by: Wang, Han2 <[email protected]> * address comment Signed-off-by: Wang, Han2 <[email protected]> --------- Signed-off-by: Wang, Han2 <[email protected]>

@pure

#4941) * extend type checking to mark extern function call that returns enum as compile time constant Signed-off-by: Wang, Han2 <[email protected]> * use @pure to annotate extern function that should be treated as compiled-time constant update reference output Signed-off-by: Wang, Han2 <[email protected]> * address comment Signed-off-by: Wang, Han2 <[email protected]> --------- Signed-off-by: Wang, Han2 <[email protected]>

@pure

#4941) * extend type checking to mark extern function call that returns enum as compile time constant Signed-off-by: Wang, Han2 <[email protected]> * use @pure to annotate extern function that should be treated as compiled-time constant update reference output Signed-off-by: Wang, Han2 <[email protected]> * address comment Signed-off-by: Wang, Han2 <[email protected]> --------- Signed-off-by: Wang, Han2 <[email protected]>

hanw requested review from ChrisDodd and kfcripps October 3, 2024 23:06

hanw force-pushed the hanw/extern-function-compiled-time-constant branch 2 times, most recently from 4d14507 to e3f7a1b Compare October 3, 2024 23:12

hanw requested a review from grg October 3, 2024 23:13

grg approved these changes Oct 3, 2024

View reviewed changes

kfcripps previously requested changes Oct 3, 2024

View reviewed changes

kfcripps requested a review from vlstill October 3, 2024 23:58

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Oct 3, 2024

extend type checking to mark extern function call that returns enum a…

8d45193

…s compile time constant Signed-off-by: Wang, Han2 <[email protected]>

hanw force-pushed the hanw/extern-function-compiled-time-constant branch from ccfe47b to c6a3eb1 Compare October 4, 2024 03:15

asl reviewed Oct 4, 2024

View reviewed changes

use @pure to annotate extern function that should be treated as compi…

3d42fbb

…led-time constant update reference output Signed-off-by: Wang, Han2 <[email protected]>

hanw force-pushed the hanw/extern-function-compiled-time-constant branch from 858d62a to 3d42fbb Compare October 4, 2024 05:59

ChrisDodd approved these changes Oct 4, 2024

View reviewed changes

address comment

03aeb18

Signed-off-by: Wang, Han2 <[email protected]>

hanw force-pushed the hanw/extern-function-compiled-time-constant branch from 131b373 to 03aeb18 Compare October 4, 2024 22:58

hanw enabled auto-merge October 5, 2024 03:48

hanw requested a review from kfcripps October 5, 2024 03:49

kfcripps reviewed Oct 5, 2024

View reviewed changes

Merge branch 'main' into hanw/extern-function-compiled-time-constant

bda6f6a

hanw added this pull request to the merge queue Oct 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024

hanw added this pull request to the merge queue Oct 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024

fruffy added this pull request to the merge queue Oct 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024

hanw added this pull request to the merge queue Oct 8, 2024

Merged via the queue into main with commit d0b05cf Oct 8, 2024
18 checks passed

hanw deleted the hanw/extern-function-compiled-time-constant branch October 8, 2024 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend type checking to mark extern function call that returns enum a… #4941

extend type checking to mark extern function call that returns enum a… #4941

hanw commented Oct 3, 2024

grg Oct 3, 2024

hanw Oct 4, 2024

kfcripps left a comment

hanw commented Oct 4, 2024

asl Oct 4, 2024

hanw Oct 4, 2024

hanw commented Oct 4, 2024

ChrisDodd Oct 4, 2024

kfcripps Oct 5, 2024

hanw Oct 5, 2024 •

edited

Loading

kfcripps Oct 5, 2024

ChrisDodd Oct 6, 2024 •

edited

Loading

hanw Oct 6, 2024

ChrisDodd Oct 7, 2024

hanw commented Oct 8, 2024

fruffy commented Oct 8, 2024

fruffy commented Oct 8, 2024

		@@ -16,6 +16,8 @@ limitations under the License.

		#include <ostream>

		#include <boost/functional/hash.hpp>

extend type checking to mark extern function call that returns enum a… #4941

extend type checking to mark extern function call that returns enum a… #4941

Conversation

hanw commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfcripps left a comment

Choose a reason for hiding this comment

hanw commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDodd Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw commented Oct 8, 2024

fruffy commented Oct 8, 2024

fruffy commented Oct 8, 2024

hanw Oct 5, 2024 •

edited

Loading

ChrisDodd Oct 6, 2024 •

edited

Loading