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

Make don't care args action-local when used in actions #4817

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

kfcripps
Copy link
Contributor

It may be easier for some passes to process action-locals that are actually action-local variables instead of control-local variables in the IR. Also note that we already do this in the DontcareArgs pass for functions, but not actions for some reason:

    const IR::Node *postorder(IR::Function *function) override {
        IR::IndexedVector<IR::StatOrDecl> body;
        for (auto d : toAdd) body.push_back(d);
        body.append(function->body->components);
        function->body = new IR::BlockStatement(function->body->srcInfo, body);
        toAdd.clear();
        return function;
    }

On main branch, the output of --top4 RemoveDontcareArgs for the attached test is:

struct S {
    bit<64> f;
}

control C(inout S s) {
    bit<64> arg;
    @name("d") action d_0(@name("b") out bit<64> b_0) {
        b_0 = 64w4;
    }
    @name("foo") action foo_0() {
        d_0(arg);
    }
    @name("t") table t_0 {
        actions = {
            foo_0();
        }
        default_action = foo_0();
    }
    apply {
        t_0.apply();
    }
}

control proto(inout S s);
package top(proto p);
top(C()) main;

and on this branch:

struct S {
    bit<64> f;
}

control C(inout S s) {
    @name("d") action d_0(@name("b") out bit<64> b_0) {
        b_0 = 64w4;
    }
    @name("foo") action foo_0() {
        bit<64> arg;
        d_0(arg);
    }
    @name("t") table t_0 {
        actions = {
            foo_0();
        }
        default_action = foo_0();
    }
    apply {
        t_0.apply();
    }
}

control proto(inout S s);
package top(proto p);
top(C()) main;

@fruffy Is there an easy way to test the output of a specific pass? This will get cleaned up by later passes so the change is not visible in the attached reference outputs.

@kfcripps kfcripps requested review from asl and fruffy July 18, 2024 21:28
@fruffy
Copy link
Collaborator

fruffy commented Jul 18, 2024

@fruffy Is there an easy way to test the output of a specific pass? This will get cleaned up by later passes so the change is not visible in the attached reference outputs.

Not exactly straightforward but take a look at https://github.com/p4lang/p4c/blob/main/test/gtest/strength_reduction.cpp

@kfcripps
Copy link
Contributor Author

@fruffy I was hoping for something that'd allow me to just pass --top4 RemoveDontcareArgs to the compiler and view the generated IR. Given that we'd need to add code to DontcareArgs to conditionally make this transformation (if I understand the referenced gtest correctly), I'd prefer to not add such gtests for this.

@kfcripps
Copy link
Contributor Author

kfcripps commented Jul 18, 2024

I was hoping for something that'd allow me to just pass --top4 RemoveDontcareArgs to the compiler and view the generated IR

We could probably modify run-p4-sample.py and related files to support these kinds of tests.

@fruffy
Copy link
Collaborator

fruffy commented Jul 19, 2024

I was hoping for something that'd allow me to just pass --top4 RemoveDontcareArgs to the compiler and view the generated IR

We could probably modify run-p4-sample.py and related files to support these kinds of tests.

It might be fairly involved to extend run-p4-sample to support this broadly across a variety of passes. I'd guess you'd have to add a custom test with a custom reference file there.

With the gtest you could just expect the IR to have a specific structure after your pass. The input to the gtest is just a P4 program.

@ChrisDodd
Copy link
Contributor

@fruffy I was hoping for something that'd allow me to just pass --top4 RemoveDontcareArgs to the compiler and view the generated IR.

Well, you can just run the .test script with --top4 RemoveDontcareArgs and it will (should) dump the P4 program after that pass(es)

@kfcripps
Copy link
Contributor Author

kfcripps commented Jul 19, 2024

We could probably modify run-p4-sample.py and related files to support these kinds of tests.

On second thought, this would be a bad idea as it would break as soon as someone would try adding a new RemoveDontcareArgs pass for some reason.

With the gtest you could just expect the IR to have a specific structure after your pass. The input to the gtest is just a P4 program.

It looks like https://github.com/p4lang/p4c/blob/main/test/gtest/strength_reduction.cpp is just running the entire set of frontend passes twice (once with some flag passed to the strength reduction pass) and checking the final output each time. It is not just running the strength reduction pass only and checking its exact output. Is my understanding not correct?

If my understanding is correct, are you suggesting to create and use my own version of FrontendTestCase::create(), which creates and runs a custom frontend that only runs the RemoveDontcareArgs pass (maybe also type inference first) instead of all of the frontend passes?

@kfcripps kfcripps force-pushed the localize-action-dont-care-args branch 2 times, most recently from 61d919a to 1f956c2 Compare July 19, 2024 10:58
@kfcripps
Copy link
Contributor Author

@fruffy I think https://github.com/p4lang/p4c/blob/main/test/gtest/frontend_test.cpp was closer to what I needed. Added the gtest.

@kfcripps kfcripps force-pushed the localize-action-dont-care-args branch from 0049337 to 24a8a02 Compare July 19, 2024 11:03
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Jul 19, 2024
@kfcripps kfcripps requested a review from ChrisDodd July 23, 2024 14:11
@@ -44,6 +44,14 @@ class DontcareArgs : public Transform, public ResolutionContext {
toAdd.clear();
return function;
}
const IR::Node *postorder(IR::P4Action *action) override {
IR::IndexedVector<IR::StatOrDecl> body;
for (auto d : toAdd) body.push_back(d);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (auto d : toAdd) body.push_back(d);
for (auto d : toAdd) {
body.push_back(d);
}

might be easier to read. This code was copied from the function segment, maybe change that piece of code, too?

@@ -0,0 +1,115 @@
#include <gtest/gtest.h>
Copy link
Collaborator

@fruffy fruffy Jul 24, 2024

Choose a reason for hiding this comment

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

This is nice! We should definitely have more tests like these...

@kfcripps kfcripps force-pushed the localize-action-dont-care-args branch from 24a8a02 to 859b705 Compare July 24, 2024 22:19
@kfcripps kfcripps enabled auto-merge July 24, 2024 22:20
@kfcripps kfcripps added this pull request to the merge queue Jul 24, 2024
Merged via the queue into p4lang:main with commit 2319fc4 Jul 24, 2024
17 checks passed
@kfcripps kfcripps deleted the localize-action-dont-care-args branch July 24, 2024 23:59
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

Successfully merging this pull request may close these issues.

3 participants