Remove apply() calls to redundant parsers #3356

bleibig · 2022-05-24T20:58:24Z

A parser with an unconditional accept in its start state is
effectively a redundant/dummy parser and can be eliminated when it is
called as a subparser. This removes the apply() calls to these
subparsers, which also removes the generation of the setInvalid() call
in resetHeaders.

frontends/CMakeLists.txt

mihaibudiu · 2022-05-24T21:20:43Z

frontends/p4/redundantParsers.h

+
+}
+
+#endif


cpplint will want this to be followed by a comment

mihaibudiu · 2022-05-24T21:21:22Z

frontends/p4/redundantParsers.h

+limitations under the License.
+*/
+
+#ifndef _P4_REDUNDANT_PARSERS_H


please use the same format for the macro as in other frontend files; otherwise cpplint will be upset.

mihaibudiu · 2022-05-24T21:21:30Z

frontends/p4/redundantParsers.h

@@ -0,0 +1,63 @@
+/*
+Copyright 2013-present Barefoot Networks, Inc.


why not 2022?

mihaibudiu · 2022-05-24T21:22:02Z

frontends/p4/redundantParsers.h

+ *  state, and put them in redundantParsers.
+ */
+class FindRedundantParsers : public Inspector {
+    std::set<cstring> &redundantParsers;


why string and not IR::P4Parser?

I tried using IR::P4Parser *, it didn't quite work because the pointers discovered in RemoveRedundantParsers did not match those found earlier, despite having the same name. Is there supposed to be a single instance of every declaration in the IR?

In a transform you should use getOriginal(), there are lots of examples in the code

mihaibudiu · 2022-05-24T21:23:21Z

frontends/p4/redundantParsers.h

+    RemoveRedundantParsers(TypeMap *typeMap)
+        : PassManager {
+                new FindRedundantParsers(redundantParsers),
+                new EliminateSubparserCalls(redundantParsers, typeMap)


The typeMap will always be empty the way you inserted this pass.
Don't you need to invoke the typechecker first?

If you do you will need a handle to the referenceMap too.

I changed it to run before ClearTypeMap. Though from my testing, even before the type map was not empty when this pass was run.

mihaibudiu · 2022-05-24T21:25:53Z

frontends/p4/redundantParsers.cpp

+}
+
+const IR::Node *EliminateSubparserCalls::postorder(IR::MethodCallStatement *mcs) {
+    auto method = mcs->methodCall->method;


There's a helper class called MethodInstance which can help with this.
It will return an ApplyMethod object for a parser invocation.

I tried using that, but I was hitting some assertion failures. Maybe there are still some issues with MethodInstance, I can look into that.

bleibig · 2022-06-10T20:07:37Z

I've updated the code to use MethodInstance. It required adding some extra checks to make sure it doesn't cause an error on calls that are not .apply(). I was also able to change the set tracking the parsers to use IR::P4Parser as well.

mihaibudiu · 2022-06-11T08:31:36Z

frontends/p4/redundantParsers.cpp

@@ -0,0 +1,66 @@
+/*
+Copyright 2013-present Barefoot Networks, Inc.


Did you really wait 9 years to subit this patch?

mihaibudiu · 2022-06-14T01:20:55Z

frontends/p4/redundantParsers.cpp

+}
+
+const IR::Node *EliminateSubparserCalls::postorder(IR::MethodCallStatement *mcs) {
+    if (mcs->methodCall->method->type->is<IR::Type_Unknown>()) return mcs;


In general the type field of an expression is not to be trusted unless you just set it up yourself (probably by calling typeChecking with a 'true' flag).

Updated to run type checking before the new passes

mihaibudiu · 2022-06-14T01:22:05Z

frontends/p4/redundantParsers.h

+ public:
+    explicit RemoveRedundantParsers(ReferenceMap *refMap)
+        : PassManager {
+                new FindRedundantParsers(redundantParsers),


If you want the 'type' field to be correct then you have to insert a call to typeChecking here. But it's probably simpler in that case to pass the computed typeMap annd use MethodInstance with a typeMap argument.

A parser with an unconditional accept in its start state is effectively a redundant/dummy parser and can be eliminated when it is called as a subparser. This removes the apply() calls to these subparsers, which also removes the generation of the setInvalid() call in resetHeaders.

Also change the parser tracking to use IR::P4Parser pointers

Also update copyright fields

bleibig · 2022-06-24T19:43:01Z

It looks like the one CI failure is from an internal build system issue, otherwise it's passing. Is this ready to merge in?

mihaibudiu requested changes May 24, 2022

View reviewed changes

bleibig force-pushed the bleibig/redundant-parsers branch from 00f1389 to 13cb8ca Compare June 9, 2022 20:14

bleibig requested a review from mihaibudiu June 13, 2022 17:08

mihaibudiu requested changes Jun 14, 2022

View reviewed changes

bleibig added 4 commits June 16, 2022 16:39

Fix cpplint errors

ec96b7a

Rewrite EliminateSubparserCalls to use MethodInstance

bdf5807

Also change the parser tracking to use IR::P4Parser pointers

Run type checking before removing redundant parsers

13842c2

Also update copyright fields

bleibig force-pushed the bleibig/redundant-parsers branch from 13cb8ca to 13842c2 Compare June 17, 2022 23:15

bleibig requested a review from mihaibudiu June 21, 2022 21:35

mihaibudiu approved these changes Jun 22, 2022

View reviewed changes

mihaibudiu merged commit ad5b4a2 into p4lang:main Jun 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove apply() calls to redundant parsers #3356

Remove apply() calls to redundant parsers #3356

bleibig commented May 24, 2022

mihaibudiu May 24, 2022

bleibig Jun 3, 2022

mihaibudiu May 24, 2022

bleibig Jun 3, 2022

mihaibudiu May 24, 2022

bleibig Jun 3, 2022

mihaibudiu May 24, 2022

bleibig Jun 3, 2022

mihaibudiu Jun 3, 2022

mihaibudiu May 24, 2022

mihaibudiu May 24, 2022

bleibig Jun 3, 2022

mihaibudiu May 24, 2022

bleibig Jun 3, 2022

bleibig commented Jun 10, 2022

mihaibudiu Jun 11, 2022

bleibig Jun 21, 2022

mihaibudiu Jun 14, 2022

bleibig Jun 21, 2022

mihaibudiu Jun 14, 2022

bleibig commented Jun 24, 2022

		@@ -0,0 +1,63 @@
		/*
		Copyright 2013-present Barefoot Networks, Inc.

		@@ -0,0 +1,66 @@
		/*
		Copyright 2013-present Barefoot Networks, Inc.


		}

		#endif

Remove apply() calls to redundant parsers #3356

Remove apply() calls to redundant parsers #3356

Conversation

bleibig commented May 24, 2022

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

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

bleibig commented Jun 10, 2022

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

bleibig commented Jun 24, 2022