Fix def_use/SimplifyDefUse to deal with extern methods properly #1284

ChrisDodd · 2018-05-17T06:44:41Z

Track extern method calls, assuming that any call to any
non-abstract method might trigger any abstract method implementation
local_copyprop fixed for the same assumption.

mihaibudiu

This code is making some assumptions about when abstract methods may be called. An abstract method can be called even asynchronously, i.e., not by the P4 dataplane program, so it should not be able to reach into its environment at all.

Maybe we need to formalize the semantics of abstract methods. One possibility would be to consider adding a special annotation @synchronous(caller) to indicate that a specific abstract method can only be called by a specific extern method and never at some other time. For these methods we could allow perhaps capturing the environment.

mihaibudiu · 2018-05-17T15:56:16Z

frontends/p4/simplifyDefUse.cpp


    // we control the traversal order manually, so we always 'prune()'
    // (return false from preorder)

    bool preorder(const IR::ParserState* state) override {
-        LOG3("FU Visiting " << dbp(state));
+        LOG3("FU Visiting state " << state->name);


One thing that dbp does which I found handy is to print the ID of the node besides the name. That makes it easy to cross-reference with a dump of a program generated using --top4 -v.

I find the id rarely useful, and dbp also just prints the id and type and none of the contents of the node (like the state name), making it mostly useless in my experience. But of course, debugging methods differ, so YMMV.

mihaibudiu · 2018-05-17T16:04:58Z

frontends/p4/simplifyDefUse.cpp

        } else if (mcd.instance->isApply()) {
            auto am = mcd.instance->to<ApplyMethod>();
            if (am->isTableApply()) {
                auto table = am->object->to<IR::P4Table>();
                callee = table;
            }
-        }
+        } else if (auto em = mcd.instance->to<ExternMethod>()) {


This code seems to be the same as in def_use.cpp. Maybe it can be refactored to be written only once?

mihaibudiu · 2018-05-17T16:10:56Z

midend/local_copyprop.cpp

+                    // method in the object, so act as if all of them are applied
+                    LOG3("extern method call " << name);
+                    name = obj + '.';
+                    for (auto it = methods.upper_bound(name); it != methods.end(); ++it) {


I don't think this is conservative enough. The virtual methods could also be invoked asynchronously at any time, e.g., by a request from the control-plane. We have not agreed that abstract methods are only invoked when some other extern method is invoked.

Also, consider action profiles: the action selector will be invoked when a table is invoked, without any other method being called explicitly.

So I think you should always consider the abstract methods as "live" and scan them. You know nothing about the context they execute in, though; they could all execute, or one of them could execute multiple times. This code should not be necessary, you should always visit them.

mihaibudiu · 2018-05-17T16:12:33Z

testdata/p4_16_samples/virtual3.p4

+
+    Virtual() cntr = {  // implementation
+        bit<16> f(in bit<16> ix) {  // abstract method implementation
+            return (ix + local);


@ChrisDodd : I have an issue filed to forbid access to the context in an abstract method. We cannot give a semantics to this code. I don't think we should allow this.

Unfortunately we can't do that -- the whole point of abstract methods in per-control instances is to get access to the context -- if we disallow that, we might as well require all externs instances be global.

Do you need an annotation here?

The @synchronous annotation was added in to the abstract method declaration in the extern above.

ChrisDodd · 2018-05-17T16:42:32Z

I think what we really need is a hook for backends to provide per-extern (and per method) information about when the method(s) might be invoked, so as to allow proper flow analysis. So this is incomplete, but an improvement over the existing code which assumed that abstract methods could never be called in the middle of the control apply block.

mihaibudiu · 2018-05-17T16:58:38Z

What is wrong with my proposal for a @synchronous annotation?

ChrisDodd · 2018-05-17T21:08:46Z

I don't think @synchronous is enough. We could create a little language in annotations -- annotate each extern method with the set of other methods it could invoke. If an asynchronous method could be invoked anywhere, I'm not sure how to deal with it in these passes, however.

ChrisDodd · 2019-01-03T19:12:25Z

So I've updated the code to use @synchronous annotations to indicate which methods might call abstract methods, so accesses to in-scope vars can be anayzed a bit better. I would like to check this in as it fixes some problems we see with dead-code eliminating metadata fields that are only used by abstract method implementations. @mbudiu-vmw any other comments?

mihaibudiu · 2019-01-03T19:34:12Z

On vacation this week with no laptop

mihaibudiu · 2019-01-04T08:43:44Z

frontends/common/resolveReferences/resolveReferences.cpp

@@ -404,10 +404,15 @@ void ResolveReferences::postorder(const IR::Type_Method *t) {

 bool ResolveReferences::preorder(const IR::Type_Extern *t) {
    refMap->usedName(t->name.name);
-    addToContext(t->typeParameters); return true; }
+    // FIXME -- should the typeParamters be part of the extern's scope?


I don’t think it will work without them.

Currently they're not part of the Type_Extern's scope, so need to be added as a separate scope (which is what this part of the change does)

mihaibudiu · 2019-01-04T08:46:42Z

frontends/p4/def_use.cpp

@@ -932,3 +934,13 @@ bool ComputeWriteSet::preorder(const IR::MethodCallStatement* statement) {
 }

 }  // namespace P4
+
+// functions for calling from gdb
+void dump(const P4::StorageLocation *s) { std::cout << *s << std::endl; }


If these implement IHasDbPrint there should already exist a dump, but indeed gdb sometimes has trouble finding the dump.

Yes, gdb seems to be unable to deal with virtual base classes properly (or at least to be able to call function overloads based on them), so I ended up adding all these overloads to make them callable.

mihaibudiu · 2019-01-04T08:49:34Z

frontends/p4/methodInstance.h

@@ -129,6 +129,11 @@ class ExternMethod final : public MethodInstance {
    const IR::Method*      method;
    const IR::Type_Extern* originalExternType;    // type of object method is applied to
    const IR::Type_Extern* actualExternType;      // with type variables substituted
+
+    /// Set of IR::Method and IR::Function objects that may be called by this method.
+    // If this method is abstract, will consist of (just) the concrete implemenation,


midend/local_copyprop.cpp

mihaibudiu · 2019-01-04T08:56:44Z

testdata/p4_16_samples/virtual3.p4

+
+    Virtual() cntr = {  // implementation
+        bit<16> f(in bit<16> ix) {  // abstract method implementation
+            return (ix + local);


Do you need an annotation here?

mihaibudiu · 2019-01-04T08:57:45Z

testdata/p4_16_samples_outputs/array_field1-midend.p4

@@ -6,18 +6,29 @@ extern bit<1> f(inout bit<1> x, in bit<1> b);
 control c(out H[2] h);
 package top(c _c);
 control my(out H[2] s) {
+    bit<32> a_0;


These changes suggest that some analysis has become less precise.

The code here changed because we're now assuming that the unknown method call f might depend on the locals. However, f is a global extern function, so we should probably assume it only depends on/modifies things passed to it explicitly.

@synchronized

- Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods

mihaibudiu

I thought a bit about it, and i think the way we should do some of this is to have an abstract base class for the current architecture which can, for example, tell you which functions are pure (e.g. Hash). The architecture should be initialized very early in the compilation.

ChrisDodd · 2019-01-06T21:36:47Z

IMO we should be doing more in the architecture P4 file to describe how the architecture works -- perrhaps (simplified) bodies for extern methods and functions that summarize what they do and can be analyzed by common frontend/midend passes to drive their transformations. I'm not a big fan of duplicating stuff between C++ classes and architecture files in a way that needs to be carefully managed but is subject to breakage when someone makes a change -- all dependencies of this nature should be strongly delineated by automatic tests that will diagnose the problem automatically when someone breaks something.

…ly (#1284)" This reverts commit 2d00ef6.

…ly (#1284)" (#1650) This reverts commit 2d00ef6.

@synchronized

- Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods

@synchronized

- Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods - adjust virtual.p4 test to reflect problematic case of non-synchronous function

@synchronized

- Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods - adjust virtual.p4 test to reflect problematic case of non-synchronous function

@synchronized

- Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods - adjust virtual.p4 test to reflect problematic case of non-synchronous function

@synchronized

… (#1652) * Fix def_use/SimplifyDefUse to deal with extern methods properly (#1284) - Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods - adjust virtual.p4 test to reflect problematic case of non-synchronous function

apinski-cavium · 2022-12-30T00:26:13Z

I just noticed @synchronous is not even documented anywhere except here and in the source of p4c.
Seems like there could be some user documentation missing here ...

mihaibudiu · 2023-01-06T22:57:17Z

@apinski-cavium we have long wanted to do that in the spec, but never got around to it: p4lang/p4-spec#943

ChrisDodd requested a review from mihaibudiu May 17, 2018 06:44

mihaibudiu requested changes May 17, 2018

View reviewed changes

ChrisDodd force-pushed the cdodd-virtfn branch from 67a7f58 to 9a4ee25 Compare May 17, 2018 16:34

mihaibudiu mentioned this pull request Sep 4, 2018

LocalCopyProp fix for abstract functions #1468

Closed

ChrisDodd force-pushed the cdodd-virtfn branch from 9a4ee25 to b68210d Compare December 10, 2018 17:03

ChrisDodd force-pushed the cdodd-virtfn branch 2 times, most recently from 878020d to e4b7a08 Compare December 21, 2018 01:28

ChrisDodd force-pushed the cdodd-virtfn branch 4 times, most recently from 0d8cdc3 to b491781 Compare January 3, 2019 17:20

ChrisDodd force-pushed the cdodd-virtfn branch 3 times, most recently from 2417681 to 15d9933 Compare January 3, 2019 20:47

mihaibudiu requested changes Jan 4, 2019

View reviewed changes

ChrisDodd force-pushed the cdodd-virtfn branch 5 times, most recently from 93f070b to 0c7cf9c Compare January 4, 2019 23:26

ChrisDodd force-pushed the cdodd-virtfn branch from 0c7cf9c to fcf1062 Compare January 5, 2019 01:55

mihaibudiu approved these changes Jan 5, 2019

View reviewed changes

ChrisDodd merged commit 2d00ef6 into p4lang:master Jan 6, 2019

ChrisDodd pushed a commit that referenced this pull request Jan 8, 2019

Revert "Fix def_use/SimplifyDefUse to deal with extern methods proper…

db5771f

…ly (#1284)" This reverts commit 2d00ef6.

ChrisDodd mentioned this pull request Jan 8, 2019

Revert "Fix def_use/SimplifyDefUse to deal with extern methods proper… #1650

Merged

ChrisDodd added a commit that referenced this pull request Jan 8, 2019

Revert "Fix def_use/SimplifyDefUse to deal with extern methods proper…

128762f

…ly (#1284)" (#1650) This reverts commit 2d00ef6.

jafingerhut mentioned this pull request Jul 2, 2019

Write document on P4_16 language issues around externs that seem most naturally written with named field references p4lang/p4-spec#766

Closed

mihaibudiu mentioned this pull request Nov 13, 2020

Abstract/virtual methods for extern objects p4lang/p4-spec#561

Closed

9 tasks

ChrisDodd deleted the cdodd-virtfn branch March 7, 2024 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix def_use/SimplifyDefUse to deal with extern methods properly #1284

Fix def_use/SimplifyDefUse to deal with extern methods properly #1284

ChrisDodd commented May 17, 2018

mihaibudiu left a comment

mihaibudiu May 17, 2018

ChrisDodd May 17, 2018

mihaibudiu May 17, 2018

mihaibudiu May 17, 2018

mihaibudiu May 17, 2018

ChrisDodd May 17, 2018

mihaibudiu Jan 4, 2019

ChrisDodd Jan 5, 2019

ChrisDodd commented May 17, 2018 •

edited

mihaibudiu commented May 17, 2018

ChrisDodd commented May 17, 2018

ChrisDodd commented Jan 3, 2019

mihaibudiu commented Jan 3, 2019

mihaibudiu Jan 4, 2019

ChrisDodd Jan 5, 2019

mihaibudiu Jan 4, 2019

ChrisDodd Jan 5, 2019

mihaibudiu Jan 4, 2019

mihaibudiu Jan 4, 2019

mihaibudiu Jan 4, 2019

ChrisDodd Jan 4, 2019

mihaibudiu left a comment

ChrisDodd commented Jan 6, 2019

apinski-cavium commented Dec 30, 2022

mihaibudiu commented Jan 6, 2023

Fix def_use/SimplifyDefUse to deal with extern methods properly #1284

Fix def_use/SimplifyDefUse to deal with extern methods properly #1284

Conversation

ChrisDodd commented May 17, 2018

mihaibudiu 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

Choose a reason for hiding this comment

ChrisDodd commented May 17, 2018 • edited

mihaibudiu commented May 17, 2018

ChrisDodd commented May 17, 2018

ChrisDodd commented Jan 3, 2019

mihaibudiu commented Jan 3, 2019

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

mihaibudiu left a comment

Choose a reason for hiding this comment

ChrisDodd commented Jan 6, 2019

apinski-cavium commented Dec 30, 2022

mihaibudiu commented Jan 6, 2023

ChrisDodd commented May 17, 2018 •

edited