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

Run def-use analysis again after inlining #3591

Merged
merged 6 commits into from
Oct 24, 2022

Conversation

mihaibudiu
Copy link
Contributor

Inlining leads to many optimization opportunities.
Doing this has uncovered a few bugs in the analysis itself, which should be fixed in this PR.
Unfortunately many reference outputs change.

Mihai Budiu added 5 commits October 21, 2022 15:13
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@mihaibudiu
Copy link
Contributor Author

test-tools has failed, but I don't know how to figure out what the problem is.

@fruffy
Copy link
Collaborator

fruffy commented Oct 22, 2022

This actually fixes a bug in the generation of the JSON for the behavioral model (#3508). Not entirely sure why, but only the XFails here need to be updated.

@fruffy fruffy linked an issue Oct 22, 2022 that may be closed by this pull request
@jafingerhut
Copy link
Contributor

jafingerhut commented Oct 22, 2022

Some of the expected -frontend and -midend output files appear to get quite a lot shorter. Is it worth checking whether they contain enough that the test program and expected output file still "cover" the compiler functionality we expect? I know that might be a tedious task, and I can help with some of them in that regard if it is deemed important.

For example, look at the changes in this PR for the file decl2-frontend.p4. Is there enough left to verify the compiler is not mis-transforming the input program?

Or perhaps gauntlet is checking every intermediate step during CI here, so we don't need to worry about this?

@@ -0,0 +1,12 @@
void x() {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these expected output files with "FrontEnd_41_SimplifyDefUse" be in this PR? I don't see any input P4 program in the p4c repo that corresponds to them. Perhaps added into this PR by mistake due to some local new test P4 program you have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have deleted these.
In principle gauntlet checks equivalence of the code, so it should flag any file where the semantics changes.
But it does not check all files, and not between all compilation points.
@fruffy what are the representations checked? We should document this somewhere.
I have reviewed some -frontend files, but not all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gauntlet currently only checks p4test's front and mid end. I have also disabled parser checking for Gauntlet (the missing implementation for extract causes issues). The only pass that is skipped is FlattenHeaderUnion. I can try to add a README on this.

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@@ -21,13 +21,13 @@ struct parsed_packet_t {

struct local_metadata_t {
short _s0;
@field_list(0)
@field_list(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why there is a space being added here?

hd_12.setInvalid();
h_0.h2[0] = hd_12;
Copy link
Collaborator

@fruffy fruffy Oct 22, 2022

Choose a reason for hiding this comment

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

Some of these seem quite aggressive. Gauntlet does not complain but I will try to check these manually. Intuitively, it makes sense since all these headers are set invalid.

Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

@fruffy
Copy link
Collaborator

fruffy commented Oct 24, 2022

Do you want to merge this first before #3606?

@mihaibudiu mihaibudiu merged commit e60cb8a into p4lang:main Oct 24, 2022
@mihaibudiu mihaibudiu deleted the def-use-bug branch October 24, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants