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

Compiler Bug: Expression E write set already set #4500

Closed
kfcripps opened this issue Mar 5, 2024 · 4 comments · Fixed by #4797
Closed

Compiler Bug: Expression E write set already set #4500

kfcripps opened this issue Mar 5, 2024 · 4 comments · Fixed by #4797
Assignees
Labels
bug This behavior is unintended and should be fixed. core Topics concerning the core segments of the compiler (frontend, midend, parser) regression Code that previously compiled correctly either no longer compiles or produces invalid results.

Comments

@kfcripps
Copy link
Contributor

kfcripps commented Mar 5, 2024

Compiling the following P4 code:

enum E {
    e0
}

extern void bar(E x);

extern void baz();

void foo(E y) {
    bar(y);
    if (y == E.e0) baz();
}

control c() {
    apply {
        foo(E.e0);
    }
}

control proto();
package top(proto p);

top(c()) main;

results in:

Compiler Bug: tmp.p4(17): Expression E write set already set
        foo(E.e0);
            ^

This bug has been introduced somewhere in between 812722f and 5f9cb55.

@kfcripps kfcripps added the bug This behavior is unintended and should be fixed. label Mar 5, 2024
@kfcripps
Copy link
Contributor Author

kfcripps commented Mar 5, 2024

Introduced by e60cb8a.

@kfcripps kfcripps added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Mar 5, 2024
@kfcripps
Copy link
Contributor Author

kfcripps commented Mar 5, 2024

I'm not sure if this is the best fix, but changing expressionWrites() in def_use.h to the following seems to work for our backend:

    void expressionWrites(const IR::Expression *expression, const LocationSet *loc) {
        CHECK_NULL(expression);
        CHECK_NULL(loc);
        LOG3(expression << dbp(expression) << " writes " << loc);
        auto it = writes.find(expression);
        if (it != writes.end()) {
            // Multiple usages of the same Expression write multiple different locations.
            // Merge the locations.
            // TODO: Re-visit this after the "correct" fix is made in the p4c upstream.
            // See https://github.com/p4lang/p4c/issues/4500.
            loc = it->second->join(loc);
        }
        writes.emplace(expression, loc);
    }

@mihaibudiu
Copy link
Contributor

mihaibudiu commented Mar 6, 2024

def_use maintains a hashmap that maps each expression to its def_use information.
What happens is that the IR DAG before def_use is invoked contains a common subtree for two different expressions; you can see the representation using: ./p4test -v --top4 FrontEnd_63 issue4500.p4

This is the relevant fragment:

      <MethodCallStatement>(10520)
        <MethodCallExpression>(10521)
          <PathExpression>(10525)
            bar
          <Vector<Type>>(54), size=0
          <Vector<Argument>>(10527), size=1
            <Argument>(10528)
              <Member>(8710)e0
                <TypeNameExpression>(8711)
                  <Type_Name>(89)
                    E */
        bar(/* 
            <Argument>(10528)
              <Member>(8710)e0
                <TypeNameExpression>(8711)
                  <Type_Name>(89)
                    E */
E.e0);
        /* 
      <IfStatement>(10531)
        <Equ>(10532)
          <Member>(8710)e0
            <TypeNameExpression>(8711)
              <Type_Name>(89)
                E
          <Member>(10535)e0
            <TypeNameExpression>(10536)
              <Type_Name>(10540)
                E
        <MethodCallStatement>(10541) */
        if (E.e0 == E.e0) {

You can see that the subtree rooted at 8710 is used in two different expressions.
The solution to this kind of problem is to make a deep copy of the respective subtree instead of reusing it.
This can be done by either tracking the place where the subtree is reused and using a deep copy of the subtree, or by inserting a pass that converts DAGs into trees before the def-use analysis.
I will try to find some time to submit a fix this week or early next week.

@kfcripps
Copy link
Contributor Author

kfcripps commented Mar 6, 2024

@mihaibudiu Makes sense, thank you for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This behavior is unintended and should be fixed. core Topics concerning the core segments of the compiler (frontend, midend, parser) regression Code that previously compiled correctly either no longer compiles or produces invalid results.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants