Split the `start` state more conservatively when a parser contains decl initializers #4902

kfcripps · 2024-09-06T20:25:57Z

Currently, the start state always gets split when a parser contains declaration initializations. The following:

parser ...
bit<16> local = 2;
state start {
    ...
}
...

always gets split into:

parser ...
bit<16> local = 2;
state start {
    ...
    transition start_0;
}

state start_0 {
    ...
}
...

However, this is only necessary when start is reachable from some other state(s). This PR makes it so that the split only happens when start is actually reachable from some other state(s).

Coincidentally, also fixes MoveInitializers splits start state for parsers that do not need this #4901

Signed-off-by: Kyle Cripps <kyle@pensando.io>

mihaibudiu · 2024-09-06T20:29:08Z

Isn't there a pass which merges consecutive states that have no other in/out edges?
That could achieve the same effect.

Signed-off-by: Kyle Cripps <kyle@pensando.io>

kfcripps · 2024-09-06T20:35:42Z

I'd rather just avoid the split in the first place if possible (and it is easy enough to accomplish this in MoveInitializers). Some backends (and/or users) may not like when P4-visible states get merged by p4c (and such a pass would likely merge both p4c-generated and user-visible states).

mihaibudiu · 2024-09-06T20:38:29Z

frontends/p4/moveDeclarations.cpp

    if (someInitializers) {
+        // Check whether any of the parser's states loop back to the start state.


Isn't there a class which builds the reachability graph for parser states?

I'm not sure, but that might help. @fruffy @vlstill ?

./frontends/p4/parserCallGraph.h

CallGraph::reachable(start, reachable) will always include start in the set of reachable states, so this will not really help simplify things.

you can check the set of in-edge of the start state

but frankly it would be useful to understand why the results are wrong. If there is a bug in the callgraph, it would be nice to know that. If the meaning of the isCalle() function is different than expected, it should be documented. If the rule for splitting is wrong, it would be good to know that too.

@mihaibudiu Personally, I find it easier to initially analyze changes by looking at git diffs, which is why I shared the link to the diff with the C++ changes as well as corresponding updated tests outputs. If you prefer to initially analyze changes by checking out the changes locally, running the tests, and inspecting the tests failures locally, you can easily do this by recreating the changes minus tests outputs updates by fetching my fork and using git checkout commands.

but frankly it would be useful to understand why the results are wrong. If there is a bug in the callgraph, it would be nice to know that. If the meaning of the isCalle() function is different than expected, it should be documented. If the rule for splitting is wrong, it would be good to know that too.

If there's a problem with the ParserCallGraph, that is outside the scope of this PR, and I'm not volunteering to spend time debugging this. But others may feel free to do so :)

I am no longer working on this project, but I will try to find time to fix bugs that I am responsible for.

@mihaibudiu For example: 4b0ec77#diff-cd2b40893d904a1015f5d71d39014e909e148f5db324e0ebb77484e99c3cc6c7R2-R7

Split is expected in this case because start is reachable from other states. So either I'm using ParserCallGraph incorrectly, or it's not working properly.

I am no longer working on this project, but I will try to find time to fix bugs that I am responsible for.

I did not realize you were no longer working on this project. Please do not feel obligated to fix even the bugs you are responsible for. But any help is of course appreciated :)

kfcripps · 2024-09-10T23:17:02Z

@fruffy @vlstill Do you want to review this? If not I'll probably merge it soon.

fruffy

Minor comments, I am not quite sure about the toMove logic since it is split across multiple visit methods which makes it harder to follow.

fruffy · 2024-09-11T07:02:04Z

frontends/p4/moveDeclarations.cpp

+        for (const auto *state : parser->states) {
+            const auto *selectExpr = state->selectExpression;
+            if (selectExpr == nullptr) {
+                // implicit reject transition


Use continue here instead?

fruffy · 2024-09-11T07:05:28Z

frontends/p4/moveDeclarations.cpp

+        // If this is the case, then the parser's decl initializers cannot be moved
+        // to the start state. A new start state that is never looped back to must
+        // be inserted at the head of the graph.
+        for (const auto *state : parser->states) {


This probably could be a self-contained helper function which returns true if a loop is detected

I would also make the control flow of the function simpler as you would just return on once you find any transtion to start.

fruffy · 2024-09-11T07:13:01Z

frontends/p4/moveDeclarations.cpp

    return state;
 }

+const IR::Node *MoveInitializers::postorder(IR::Path *path) {
+    if (!oldStart || !loopsBackToStart || path->name != IR::ParserState::start) return path;


Suggested change

if (!oldStart || !loopsBackToStart || path->name != IR::ParserState::start) return path;

if (oldStart == nullptr || !loopsBackToStart || path->name != IR::ParserState::start) return path;

vlstill

I have just a few code quality points.

frontends/p4/moveDeclarations.cpp

vlstill · 2024-09-11T11:52:01Z

frontends/p4/moveDeclarations.cpp

+        // If this is the case, then the parser's decl initializers cannot be moved
+        // to the start state. A new start state that is never looped back to must
+        // be inserted at the head of the graph.
+        for (const auto *state : parser->states) {


I would also make the control flow of the function simpler as you would just return on once you find any transtion to start.

frontends/p4/moveDeclarations.cpp

vlstill · 2024-09-11T11:58:18Z

frontends/p4/moveDeclarations.cpp

+    oldStart = nullptr;
+    loopsBackToStart = false;


Do we need this cleanup here? Would make more sense to do the cleanup when entering the parser so that it is clear what are initial values for each packet analysis.

They are already initialized in the MoveInitializers constructor. Are you suggesting to not initialize them there at all, or to initialize them twice (in both constructor and preorder function) the first time?

I guess they are already being initialized twice for the first parser anyway, so I will move the initializations out of the constructor to avoid this.

I think initialization in preorder makes sense, whether to initialize also in constructor is up to you. Some linters may complain about unitialized class fields if the constructor does not initialize it (because they simply have no way of proving that preorder/init_apply/etc. runs before anything else that uses the var), but I think having no initializer in constructor makes a little more sense as the value is then overwritten anyway.

frontends/p4/moveDeclarations.h

vlstill · 2024-09-11T12:00:46Z

testdata/p4_16_samples/issue4901_new_start_with_start_state_loopback.p4

+parser Parser2() {
+  state start {
+    transition  accept;
+  }
+}


I'd add some initializer here just to see that there is no confusion between the two parsers.

I'd prefer to not change this test (it is the case taken from #4901) but I added a new one.

Signed-off-by: kfcripps <kyle@pensando.io> Signed-off-by: Kyle Cripps <kyle@pensando.io>

Signed-off-by: Kyle Cripps <kyle@pensando.io>

kfcripps · 2024-09-12T12:41:34Z

Thanks @vlstill @fruffy @mihaibudiu

asl · 2024-09-13T01:27:06Z

@kfcripps I think this contains UB: https://github.com/p4lang/p4c/actions/runs/10839879711/job/30081116349

asl · 2024-09-13T01:28:45Z

loopsBackToStart is not initialized

kfcripps added 2 commits September 6, 2024 13:25

More conservatively split the start state

ed624a5

Signed-off-by: Kyle Cripps <kyle@pensando.io>

Update tests outputs

a159c12

Signed-off-by: Kyle Cripps <kyle@pensando.io>

kfcripps added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Sep 6, 2024

kfcripps requested review from vlstill, fruffy and mihaibudiu September 6, 2024 20:25

appease clang-format

ac60a8b

Signed-off-by: Kyle Cripps <kyle@pensando.io>

mihaibudiu approved these changes Sep 6, 2024

View reviewed changes

fruffy reviewed Sep 11, 2024

View reviewed changes

vlstill reviewed Sep 11, 2024

View reviewed changes

kfcripps added 2 commits September 11, 2024 09:30

review suggestions

f9ee8dc

Signed-off-by: kfcripps <kyle@pensando.io> Signed-off-by: Kyle Cripps <kyle@pensando.io>

Add new test case

70c3864

Signed-off-by: Kyle Cripps <kyle@pensando.io>

kfcripps requested review from vlstill and fruffy September 11, 2024 16:32

appease cpplint

05e82b0

Signed-off-by: Kyle Cripps <kyle@pensando.io>

vlstill approved these changes Sep 12, 2024

View reviewed changes

kfcripps added this pull request to the merge queue Sep 12, 2024

Merged via the queue into p4lang:main with commit 9e77cde Sep 12, 2024
18 checks passed

kfcripps deleted the 4901-split-start-conservatively branch September 12, 2024 13:33

asl mentioned this pull request Sep 13, 2024

Revert "Split the start state more conservatively when a parser contains decl initializers" #4909

Closed

kfcripps mentioned this pull request Sep 13, 2024

Only process IR::Paths inside of IR::ParserState contexts in MoveInitializers #4910

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the `start` state more conservatively when a parser contains decl initializers #4902

Split the `start` state more conservatively when a parser contains decl initializers #4902

kfcripps commented Sep 6, 2024

mihaibudiu commented Sep 6, 2024

kfcripps commented Sep 6, 2024 •

edited

Loading

mihaibudiu Sep 6, 2024

kfcripps Sep 6, 2024

mihaibudiu Sep 6, 2024

kfcripps Sep 9, 2024

mihaibudiu Sep 9, 2024

mihaibudiu Sep 9, 2024

kfcripps Sep 9, 2024

kfcripps Sep 9, 2024

mihaibudiu Sep 9, 2024

kfcripps Sep 9, 2024

kfcripps commented Sep 10, 2024

fruffy left a comment

fruffy Sep 11, 2024

fruffy Sep 11, 2024

vlstill Sep 11, 2024

fruffy Sep 11, 2024

vlstill left a comment

vlstill Sep 11, 2024

vlstill Sep 11, 2024

kfcripps Sep 11, 2024

kfcripps Sep 11, 2024

vlstill Sep 12, 2024

vlstill Sep 11, 2024

kfcripps Sep 11, 2024

kfcripps commented Sep 12, 2024

asl commented Sep 13, 2024

asl commented Sep 13, 2024

		if (someInitializers) {
		// Check whether any of the parser's states loop back to the start state.

	if (!oldStart \|\| !loopsBackToStart \|\| path->name != IR::ParserState::start) return path;
	if (oldStart == nullptr \|\| !loopsBackToStart \|\| path->name != IR::ParserState::start) return path;

Split the start state more conservatively when a parser contains decl initializers #4902

Split the start state more conservatively when a parser contains decl initializers #4902

Conversation

kfcripps commented Sep 6, 2024

mihaibudiu commented Sep 6, 2024

kfcripps commented Sep 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfcripps commented Sep 10, 2024

fruffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfcripps commented Sep 12, 2024

asl commented Sep 13, 2024

asl commented Sep 13, 2024

Split the `start` state more conservatively when a parser contains decl initializers #4902

Split the `start` state more conservatively when a parser contains decl initializers #4902

kfcripps commented Sep 6, 2024 •

edited

Loading