Make HasTableApply a resolution context, so it could resolve declarations on its own if desired #4781

asl · 2024-07-03T22:42:47Z

asl · 2024-07-03T22:51:38Z

@ChrisDodd So, this is one case where ResolutionContext abstraction seems to slip through the cracks.

The problem is as follows:

ResolutionContext could only resolve declarations from its own context
There is one very fragile place: resolution of overloads as current code in ResolutionContext::methodArguments essentially expects that current node is a method call (so we are at call or "inside" of code required to e.g. compute arguments)
This fails miserably for the cases (like in side effects) where getDeclaration is called on something like foo->bar->baz, so the node itself is unrolled without any context. so we cannot see e.g. "bar" or "baz" here. Same pattern exists in e.g. MethodInstance::resolve and in some other places.
In case of sideeffects we are having an AssignmentStatement like foo = bar(baz) on the context and we are trying to get decl of bar, as bar is not on the context, methodArguments cannot resolve overload.

As a result, we cannot pass ResolutionContext "downstream" as DeclarationLookup, we need to always inherit context, make nested thing a ResolutionContext and ensure that necessary node is on the context. But this is very fragile as if we're having foo on the context and we call apply on baz from foo->bar->baz, then bar will be invisible to ResolutionContext. So things are quite error prone...

Any ideas how to solve this in generic and non error-prone way are welcome :)

ChrisDodd · 2024-07-03T23:04:18Z

For something like foo.bar.baz (no -> in P4), we need to look up foo in the current context, and then look up bar in the namespace of the type of foo. This is a simple lookup in a namespace -- there's no need for the context of (the type of) foo, as bar can't resolve to anything "higher up" -- it must be directly in the namespace of foo. So there should be no need for a resolution context -- you already have a namespace in which to look up bar (and similarly for baz).

ChrisDodd · 2024-07-03T23:11:55Z

frontends/p4/sideEffects.h

@@ -28,7 +28,7 @@ limitations under the License.
 namespace P4 {

 /// Checks to see whether an IR node includes a table.apply() sub-expression
-class HasTableApply : public Inspector {
+class HasTableApply : public Inspector, public ResolutionContext {


If you make HasTableApply a ResolutionContext, you don't need the refMap field in it any more -- you just use this. However, as HasTableApply is only ever called on expressions, there won't be any additional scopes to look things up in within it's visit traversal -- the innermost scope will be the same as the caller. So I don't see how this affects anything.

It is still called with refmap somewhere in the midend. And I do not want to trigger set of changes there.

And it does affect exactly due to outlined case, see

p4c/frontends/common/resolveReferences/resolveReferences.cpp

Line 183 in 676bce5

const IR::Vector<IR::Argument> *ResolutionContext::methodArguments(cstring name) const {

– it is only be able to resolve things on the context.

So, KeySideEffects cannot resolve overload, it it "further down".

Please add a comment here that this needs to be ResolutionContext so that if we are working without a P4::ReferenceMap we have the call in context for the sake of MethodInstance::resolve.

asl · 2024-07-03T23:38:57Z

For something like foo.bar.baz (no -> in P4), we need to look up foo in the current context, and then look up bar in the namespace of the type of foo.

I mean literally, where foo, bar and baz are different nodes inside compiler. Let's take KeySideEffects. Here we are having:

    const IR::Node *postorder(IR::AssignmentStatement *statement) override {
        return doStatement(statement, statement->right, getContext());
    }

Note that statement is on innermost context, but doStatement is called on statement->right as well. And it is statement->right that has HasTableApply called on:

    LOG3("Visiting " << getOriginal());
    HasTableApply hta(typeMap);
    hta.setCalledBy(this);

    (void)expression->apply(hta, ctxt);

So, we cannot have HasTableApply to use KeySideEffects as DeclarationLookup as statement->right is "invisible" in its context, so KeySideEffects cannot resolve anything that involves statement->right (it is MethodCallExpression that has MethodInstance::resolve to be called, that in turn, grabs a nested PathExpression and tries to get Declaration from the Path). And in the testcase it is add from hdr.value_3 = add(hdr.value_1, hdr.value_2) , so it cannot "see" add (as it is further down) and therefore cannot resolve the overloads.

vlstill

Wow, this is quite tricky. But I absolutely see why the call itself needs to be on the context.

We should at least add some bug check that allows catching these problems in MethodInstance. Possibly to ResolutionContext::resolveUnique (something like !decls[0]->is<IR::IFunction>() || argumets after more than one decl is found). Or possibly to MethodInstance to check somehow that the mce is in context of the refMap, but that seems both more complex to do and less general.

testdata/p4_16_samples/issue4775.p4

vlstill · 2024-07-04T05:09:35Z

frontends/p4/sideEffects.h

@@ -28,7 +28,7 @@ limitations under the License.
 namespace P4 {

 /// Checks to see whether an IR node includes a table.apply() sub-expression
-class HasTableApply : public Inspector {
+class HasTableApply : public Inspector, public ResolutionContext {


Please add a comment here that this needs to be ResolutionContext so that if we are working without a P4::ReferenceMap we have the call in context for the sake of MethodInstance::resolve.

frontends/p4/sideEffects.h

asl · 2024-07-04T05:45:11Z

We should at least add some bug check that allows catching these problems in MethodInstance.

Yes, I was thinking to BUG_CHECK in ResolutionContext::methodArguments, so to ensure that we always resolved the call - I do not think that we need to allow unresolved calls there. I was also concerned about MethodInstance::resolve itself, as it is widespread across the codebase, but looks like we are ok here as we cannot declare something "inside" MethodCallExpression / MethidCallStatement. So, the only fragile part is ResolutionContext::methodArguments and for this to work we need to ensure that we do have call on the context, so it's up to the caller to ensure.

vlstill · 2024-07-04T08:56:58Z

Yes, I was thinking to BUG_CHECK in ResolutionContext::methodArguments, so to ensure that we always resolved the call - I do not think that we need to allow unresolved calls there.

The problem here could be that methodArguments may not be called with a method at all. Just a doubly-defined varibale will probably exercise the same path (unless it results in error earlier). So I think we need to check that the (multiple) declarations found are callable before checking arguments of the call.

ChrisDodd · 2024-07-04T22:19:42Z

For something like foo.bar.baz (no -> in P4), we need to look up foo in the current context, and then look up bar in the namespace of the type of foo.

I mean literally, where foo, bar and baz are different nodes inside compiler. Let's take KeySideEffects. Here we are having:
    const IR::Node *postorder(IR::AssignmentStatement *statement) override {
        return doStatement(statement, statement->right, getContext());
    }
Note that statement is on innermost context, but doStatement is called on statement->right as well. And it is statement->right that has HasTableApply called on:
    LOG3("Visiting " << getOriginal());
    HasTableApply hta(typeMap);
    hta.setCalledBy(this);

    (void)expression->apply(hta, ctxt);
So, we cannot have HasTableApply to use KeySideEffects as DeclarationLookup as statement->right is "invisible" in its context, so KeySideEffects cannot resolve anything that involves statement->right (it is MethodCallExpression that has MethodInstance::resolve to be called, that in turn, grabs a nested PathExpression and tries to get Declaration from the Path). And in the testcase it is add from hdr.value_3 = add(hdr.value_1, hdr.value_2) , so it cannot "see" add (as it is further down) and therefore cannot resolve the overloads.

I don't see this at all. The only issue with using getContext() here when resolving in statement->right is that you lose the AssignmentStatement in the context -- it looks like the parent of the MethodCall is the containing BlockStatement. But that's ok, as an AssignmentStatement is not an INamespace so would be skipped over by resolution lookup anyway. We could use getChildContext() here instead (various other places do that) to include the AssignmentStatement in the context, but as I said, that would turn out to be irrelevant.

In fact, if you look at this example (issue4775) it has no problem finding the definition of add -- the problem is that it finds two of them and then flags that as an error, rather than doing overload resolution. I'm not sure why that is happending -- the resolve call should be looking through the available definitions and deciding which one matches the call site, and not calling resolveUnique.

asl · 2024-07-04T22:32:39Z

I'm not sure why that is happending -- the resolve call should be looking through the available definitions and deciding which one matches the call site, and not calling resolveUnique.

It is not problem of finding add: their definitions are in one of namespaces that are on the context. The problem is overload resolution. See ResolutionContext::methodArguments, as I already said few times it is very sensitive to what is on innermost context:

const IR::Vector<IR::Argument> *ResolutionContext::methodArguments(cstring name) const {
    const Context *ctxt = getChildContext();
    while (ctxt) {
        const auto *node = ctxt->original;
        const IR::MethodCallExpression *mc = nullptr;
        if (const auto *mcs = node->to<IR::MethodCallStatement>())
            mc = mcs->methodCall;
        else
            mc = node->to<IR::MethodCallExpression>();

        if (mc) {
            if (const auto *mem = mc->method->to<IR::Member>()) {
                if (mem->member == name) return mc->arguments;
            }
            if (const auto *path = mc->method->to<IR::PathExpression>()) {
                if (path->path->name == name) return mc->arguments;
            }
            break;
        }

        if (const auto *decl = node->to<IR::Declaration_Instance>()) {
            if (decl->name == name) return decl->arguments;
            if (const auto *type = decl->type->to<IR::Type_Name>()) {
                if (type->path->name == name) return decl->arguments;
            }
            if (const auto *ts = decl->type->to<IR::Type_Specialized>()) {
                if (ts->baseType->path->name == name) return decl->arguments;
            }
            break;
        }
        if (ctxt->node->is<IR::Expression>() || ctxt->node->is<IR::Type>())
            ctxt = ctxt->parent;
        else
            break;
    }
    LOG4("No arguments found for calling " << name << " in " << ctxt->node);

    return nullptr;
}

In the issue4775 case, the node (on the context) is an AssignmentStatement. And it is always looks "up", not down. As a result, it is never able to resolve the call arguments. To be short:

KeySideEffects cannot resolve this overload. It has AssignmentStatement on the innermost context, it does not see call at all and therefore cannot deduce the arguments for overload resolution.
As a result, HasTableApply must resolve the call and must be ResolutionContext by itself, as it has call (statement->right) on the context and can inherit context from KeySideEffects

asl · 2024-07-04T22:41:04Z

The problem here could be that methodArguments may not be called with a method at all. Just a doubly-defined varibale will probably exercise the same path (unless it results in error earlier). So I think we need to check that the (multiple) declarations found are callable before checking arguments of the call.

Oh, I see what you mean. In fact, methodCall seem to allow to be "inside" call expression, so it could go up and fine the call. So we cannot simply check that is called on a callable (though I do not know if there are cases that rely on this). What I'm suggesting is to BUG_CHECK at the end of ResolutionContext::methodArguments: we should always be able to resolve arguments.

ChrisDodd · 2024-07-04T23:25:57Z

Ah, ok, now I understand what you were talking about -- the problem being that methodArguments could not find the MethodCallExpression to get the arguments, as the wrong Visitor::Context was being used for the lookup (an earlier one that doesn't include the MethodCall).

Longer term, it might help to merge ResolutionContext into the Visitor class directly -- that way every visitor is automatically a ResolutionContext. There's no real cost to this -- Visitor already maintains the Visitor::Context chain, which is what is being used for the lookup. The main (remaining) error foot-gun is calling ->apply on an inner visitor without supplying the ctxt argument -- in general one should NEVER do that. Perhaps make that argument non-optional, so one would need to supply an explicit nullptr for a top-level visitor.

ChrisDodd · 2024-07-04T23:31:17Z

I suggest also modifying midend/simplifyKey.cpp as:

--- a/midend/simplifyKey.cpp
+++ b/midend/simplifyKey.cpp
@@ -80,9 +80,9 @@ const IR::Node *DoSimplifyKey::postorder(IR::P4Table *table) {
 const IR::Node *DoSimplifyKey::doStatement(const IR::Statement *statement,
                                            const IR::Expression *expression) {
     LOG3("Visiting " << getOriginal());
-    HasTableApply hta(refMap, typeMap);
+    HasTableApply hta(typeMap);
     hta.setCalledBy(this);
-    (void)expression->apply(hta);
+    (void)expression->apply(hta, getChildContext());
     if (hta.table == nullptr) return statement;
     auto insertions = get(toInsert, hta.table);
     if (insertions == nullptr) return statement;

which allows getting rid of the refMap in HasTableApply

asl · 2024-07-04T23:32:37Z

Ah, ok, now I understand what you were talking about -- the problem being that methodArguments could not find the MethodCallExpression to get the arguments, as the wrong Visitor::Context was being used for the lookup (an earlier one that doesn't include the MethodCall).

Exactly.

The main (remaining) error foot-gun is calling ->apply on an inner visitor without supplying the ctxt argument -- in general one should NEVER do that. Perhaps make that argument non-optional, so one would need to supply an explicit nullptr for a top-level visitor.

I was also thinking of always providing context to getDeclaration, so DeclarationLookup would require context as it is context-sensitive.

Still, there are few interesting cases (like in Evaluator or Inliner) where we are have, say, call, but we'd need to resolve something in the callee context...

ChrisDodd · 2024-07-05T01:01:14Z

I was also thinking of always providing context to getDeclaration, so DeclarationLookup would require context as it is context-sensitive.

That could work -- basically, getDeclaration becomes a free function and takes a Visitor::Context * as an argument instead of being a member of DeclarationLookup. Or could make it a member of Visitor::Context?

Still, there are few interesting cases (like in Evaluator or Inliner) where we are have, say, call, but we'd need to resolve something in the callee context...

The tricky thing here is that if you want to recurse into the callee in the same visitor, you should NOT use the context of MethodCall doing the call -- the context should be the scope where the method was found. Otherwise you would accidentally do dynamic scoping, and symbols in the callee scope would be visible and might shadow globals. This won't happen if you call UniqueNames first (which will rename things so there is no shadowing), so maybe it is not necessary to worry about this too much -- UniqueNames gets called in the frontend before we ever call the Evaluator or Inliner for precisely this purpose (mostly for Inlining, since that actually rewrites the code, and would have problems if locals could shadow globals, regardless of what happens with the Visitor::Context)

ChrisDodd · 2024-07-05T01:06:31Z

BTW, if you do include the mod to midend/simplifyKey.cpp above, you'll also need:

--- a/midend/hsIndexSimplify.cpp
+++ b/midend/hsIndexSimplify.cpp
@@ -160,7 +160,8 @@ class IsNonConstantArrayIndex : public KeyIsSimple, public Inspector {
 
 IR::Node *HSIndexContretizer::preorder(IR::P4Control *control) {
     DoSimplifyKey keySimplifier(refMap, typeMap, new IsNonConstantArrayIndex());
-    const auto *controlKeySimplified = control->apply(keySimplifier)->to<IR::P4Control>();
+    const auto *controlKeySimplified =
+        control->apply(keySimplifier, getContext())->to<IR::P4Control>();
     auto *newControl = controlKeySimplified->clone();
     IR::IndexedVector<IR::Declaration> newControlLocals;
     GeneratedVariablesMap blockGeneratedVariables;

There's that foot-gun of calling apply without the context.

asl · 2024-07-05T03:56:03Z

That could work -- basically, getDeclaration becomes a free function and takes a Visitor::Context * as an argument instead of being a member of DeclarationLookup. Or could make it a member of Visitor::Context?

Yes. Alternatively – we do have setCalledBy. Why don't we "steal" context from the caller via there? So it could be default in case of nullptr context.

ChrisDodd · 2024-07-05T04:35:00Z

That could work -- basically, getDeclaration becomes a free function and takes a Visitor::Context * as an argument instead of being a member of DeclarationLookup. Or could make it a member of Visitor::Context?

Yes. Alternatively – we do have setCalledBy. Why don't we "steal" context from the caller via there? So it could be default in case of nullptr context.

Possible, but I don't thing setCalledBy is called reliably either. However, using that when ctxt == nullptr could cover more cases.

ChrisDodd · 2024-07-05T04:37:26Z

...or we could put in BUG_CHECKs for when the setCalledBy parent context and the apply context are inconsistent. Make all the code call both properly.

asl · 2024-07-05T05:06:06Z

I removed refMap from HasTableApply and updated midend users. This also removed couple of upper-level ResolutionContext's.

As for BUG_CHECK for nullptr result inside methodArguments, it seems that P4CArchitecture.duplicatedDeclarationBug test relies on this behavior – likely it should be changed, so the diagnostics is provided elsewhere.

ChrisDodd · 2024-07-07T01:54:00Z

I think there are a number of ways we could try to make the use of ResolutionContext safer and/or clearer

Add an (optional?) const Visitor::Context * argument to all the various ResolutionContext::resolveX methods -- the default would be to use getChildContext() on the visitor as is done currently. There are probably only a couple of places where this would ever be used, but it adds flexibility.
merge ResolutionContext into Visitor (making every Visitor a ResolutionContext). This could be combined with 1, but would probably cover all the same places
merge ResolutionContext into Visitor::Context. Visitor could also be made DeclarationLookup that just forwards the various calls to its current context. This has a small cost, as it makes Visitor::Context a dynamic class, so it probably ends up requiring and extra word of storage for the vtable pointer. We allocate one on the stack for every node we're visiting in apply_visitor, so the cost is likely not too bad.

If we ever do manage to completely eliminate the reference maps, then some of the cost could be reduced.

vlstill · 2024-07-08T05:51:31Z

frontends/p4/sideEffects.h

+///
+/// Note: when working without refMap, we need to have a call on the context,
+/// so ResolutionContext could find the original declaration (invoked inside
+/// MethodInstance::resolve).


Please update the comment as there is no longer an option to work with refMap

…ions on its own if desired Fixes p4lang#4775 Signed-off-by: Anton Korobeynikov <[email protected]>

Signed-off-by: Anton Korobeynikov <[email protected]>

asl requested review from vlstill and ChrisDodd July 3, 2024 22:42

ChrisDodd reviewed Jul 3, 2024

View reviewed changes

vlstill reviewed Jul 4, 2024

View reviewed changes

asl force-pushed the 4775-fix branch 2 times, most recently from 94cae53 to 3d02b37 Compare July 4, 2024 05:51

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Jul 4, 2024

asl force-pushed the 4775-fix branch 2 times, most recently from cbf84a2 to 309a8ad Compare July 5, 2024 04:59

ChrisDodd approved these changes Jul 7, 2024

View reviewed changes

vlstill approved these changes Jul 8, 2024

View reviewed changes

Make HasTableApply a resolution context, so it could resolve declarat…

f1ea417

…ions on its own if desired Fixes p4lang#4775 Signed-off-by: Anton Korobeynikov <[email protected]>

Get rid of refMap in HasTableApply. Some cleanup while there

d777f10

Signed-off-by: Anton Korobeynikov <[email protected]>

asl force-pushed the 4775-fix branch from 309a8ad to d777f10 Compare July 8, 2024 21:01

asl enabled auto-merge July 8, 2024 21:06

asl added this pull request to the merge queue Jul 8, 2024

Merged via the queue into p4lang:main with commit 5be356c Jul 8, 2024
17 checks passed

asl deleted the 4775-fix branch July 8, 2024 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make HasTableApply a resolution context, so it could resolve declarations on its own if desired #4781

Make HasTableApply a resolution context, so it could resolve declarations on its own if desired #4781

asl commented Jul 3, 2024

asl commented Jul 3, 2024

ChrisDodd commented Jul 3, 2024

ChrisDodd Jul 3, 2024

asl Jul 3, 2024

asl Jul 3, 2024

vlstill Jul 4, 2024

asl commented Jul 3, 2024 •

edited

Loading

vlstill left a comment

vlstill Jul 4, 2024

asl commented Jul 4, 2024

vlstill commented Jul 4, 2024

ChrisDodd commented Jul 4, 2024

asl commented Jul 4, 2024 •

edited

Loading

asl commented Jul 4, 2024

ChrisDodd commented Jul 4, 2024

ChrisDodd commented Jul 4, 2024

asl commented Jul 4, 2024

ChrisDodd commented Jul 5, 2024

ChrisDodd commented Jul 5, 2024

asl commented Jul 5, 2024

ChrisDodd commented Jul 5, 2024 •

edited

Loading

ChrisDodd commented Jul 5, 2024

asl commented Jul 5, 2024

ChrisDodd commented Jul 7, 2024

vlstill Jul 8, 2024

Make HasTableApply a resolution context, so it could resolve declarations on its own if desired #4781

Make HasTableApply a resolution context, so it could resolve declarations on its own if desired #4781

Conversation

asl commented Jul 3, 2024

asl commented Jul 3, 2024

ChrisDodd commented Jul 3, 2024

ChrisDodd Jul 3, 2024

Choose a reason for hiding this comment

asl Jul 3, 2024

Choose a reason for hiding this comment

asl Jul 3, 2024

Choose a reason for hiding this comment

vlstill Jul 4, 2024

Choose a reason for hiding this comment

asl commented Jul 3, 2024 • edited Loading

vlstill left a comment

Choose a reason for hiding this comment

vlstill Jul 4, 2024

Choose a reason for hiding this comment

asl commented Jul 4, 2024

vlstill commented Jul 4, 2024

ChrisDodd commented Jul 4, 2024

asl commented Jul 4, 2024 • edited Loading

asl commented Jul 4, 2024

ChrisDodd commented Jul 4, 2024

ChrisDodd commented Jul 4, 2024

asl commented Jul 4, 2024

ChrisDodd commented Jul 5, 2024

ChrisDodd commented Jul 5, 2024

asl commented Jul 5, 2024

ChrisDodd commented Jul 5, 2024 • edited Loading

ChrisDodd commented Jul 5, 2024

asl commented Jul 5, 2024

ChrisDodd commented Jul 7, 2024

vlstill Jul 8, 2024

Choose a reason for hiding this comment

asl commented Jul 3, 2024 •

edited

Loading

asl commented Jul 4, 2024 •

edited

Loading

ChrisDodd commented Jul 5, 2024 •

edited

Loading