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.

Sign up for GitHub

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

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

Merged
merged 4 commits into from
Oct 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d45193
extend type checking to mark extern function call that returns enum a…
Oct 3, 2024
3d42fbb
use @pure to annotate extern function that should be treated as compi…
Oct 3, 2024
03aeb18
address comment
Oct 4, 2024
bda6f6a
Merge branch 'main' into hanw/extern-function-compiled-time-constant
Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions frontends/p4/typeChecking/typeCheckExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2157,12 +2157,14 @@ const IR::Node *TypeInferenceBase::postorder(const IR::MethodCallExpression *exp
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>());
}
Comment on lines 2157 to 2170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@hanw hanw Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ChrisDodd ChrisDodd Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
52 changes: 52 additions & 0 deletions testdata/p4_16_samples/hashext3.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
Copyright 2024 Intel Corp.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

enum HashAlgorithm_t {
IDENTITY,
RANDOM
}

@pure extern HashAlgorithm_t identity_hash(bool msb, bool extend);
@pure extern HashAlgorithm_t random_hash(bool msb, bool extend);

extern Hash<W> {
/// Constructor
/// @type_param W : width of the calculated hash.
/// @param algo : The default algorithm used for hash calculation.
Hash(HashAlgorithm_t algo);

/// Compute the hash for the given data.
/// @param data : The list of fields contributing to the hash.
/// @return The hash value.
W get<D>(in D data);
}

header h1_t {
bit<32> f1;
bit<32> f2;
bit<32> f3;
}

struct hdrs {
h1_t h1;
bit<16> hash;
}

control test(inout hdrs hdr) {
apply {
hdr.hash = Hash<bit<16>>(random_hash(false, false)).get(hdr.h1);
}
}
29 changes: 29 additions & 0 deletions testdata/p4_16_samples_outputs/hashext3-first.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
enum HashAlgorithm_t {
IDENTITY,
RANDOM
}

@pure extern HashAlgorithm_t identity_hash(bool msb, bool extend);
@pure extern HashAlgorithm_t random_hash(bool msb, bool extend);
extern Hash<W> {
Hash(HashAlgorithm_t algo);
W get<D>(in D data);
}

header h1_t {
bit<32> f1;
bit<32> f2;
bit<32> f3;
}

struct hdrs {
h1_t h1;
bit<16> hash;
}

control test(inout hdrs hdr) {
apply {
hdr.hash = Hash<bit<16>>(random_hash(false, false)).get<h1_t>(hdr.h1);
}
}

23 changes: 23 additions & 0 deletions testdata/p4_16_samples_outputs/hashext3-frontend.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
enum HashAlgorithm_t {
IDENTITY,
RANDOM
}

@pure extern HashAlgorithm_t identity_hash(bool msb, bool extend);
@pure extern HashAlgorithm_t random_hash(bool msb, bool extend);
extern Hash<W> {
Hash(HashAlgorithm_t algo);
W get<D>(in D data);
}

header h1_t {
bit<32> f1;
bit<32> f2;
bit<32> f3;
}

struct hdrs {
h1_t h1;
bit<16> hash;
}

29 changes: 29 additions & 0 deletions testdata/p4_16_samples_outputs/hashext3.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
enum HashAlgorithm_t {
IDENTITY,
RANDOM
}

@pure extern HashAlgorithm_t identity_hash(bool msb, bool extend);
@pure extern HashAlgorithm_t random_hash(bool msb, bool extend);
extern Hash<W> {
Hash(HashAlgorithm_t algo);
W get<D>(in D data);
}

header h1_t {
bit<32> f1;
bit<32> f2;
bit<32> f3;
}

struct hdrs {
h1_t h1;
bit<16> hash;
}

control test(inout hdrs hdr) {
apply {
hdr.hash = Hash<bit<16>>(random_hash(false, false)).get(hdr.h1);
}
}

4 changes: 4 additions & 0 deletions testdata/p4_16_samples_outputs/hashext3.p4-stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
hashext3.p4(48): [--Wwarn=unused] warning: Control test is not used; removing
control test(inout hdrs hdr) {
^^^^
[--Wwarn=missing] warning: Program does not contain a `main' module
3 changes: 3 additions & 0 deletions testdata/p4_16_samples_outputs/issue4661_pure_extern_function_const_args.p4-stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
issue4661_pure_extern_function_const_args.p4(13): [--Wwarn=mismatch] warning: baz(): constant expression in switch
switch(baz()) {
^^^^^
Loading