Skip to content

Consistently set & propagate child names#5134

Merged
asl merged 2 commits into
p4lang:mainfrom
asl:stale-child-name
Feb 24, 2025
Merged

Consistently set & propagate child names#5134
asl merged 2 commits into
p4lang:mainfrom
asl:stale-child-name

Conversation

@asl

@asl asl commented Feb 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

child_name is only updated on the context if it is non-empty. Looks like it was a ad-hoc approach to propagate child_name for Vector / IndexedVector as these do not have context and visitation of them looks like as we just unroll the vector.

However, these caused unwanted side effects in many places (and could be easily seen via e.g. dumper pass). Consider, for example, P4Parser:

void IR::P4Parser::visit_children(Visitor & v) const {
    Type_Declaration::visit_children(v);
    v.visit(type, "type");
    v.visit(constructorParams, "constructorParams");
    parserLocals.visit_children(v);
    states.visit_children(v);
}

Here the child_name inside parserLocals.visit_children(v) and states.visit_children(v) would be constructorParams and this is not something we'd want (note multiple constructorParams fields):

  [671] P4Parser name=p declid=1
    type: [30] Type_Parser name=p declid=0
      typeParameters: [5] TypeParameters
      applyParams: [26] ParameterList
        [10] Parameter name=check declid=0 direction=in
          type: [8] Type_Boolean
        [18] Parameter name=foo declid=1 direction=in
          type: [16] Type_Bits size=10 isSigned=0
        [23] Parameter name=matches declid=2 direction=out
          type: [21] Type_Boolean
    constructorParams: [153] ParameterList
    constructorParams: [490] Declaration_Variable name=val1 declid=12
      type: [2] Type_Bits size=32 isSigned=0
      initializer: [487] Cast
        type: [2] Type_Bits size=32 isSigned=0...
        expr: [486] Constant value=2 base=10
          type: [2] Type_Bits size=32 isSigned=0...
        destType: [2] Type_Bits size=32 isSigned=0...
    constructorParams: [696] ParserState name=start declid=4

Here we extend visit_children to accept a name argument and use to to pass the upper-level field name. Example above becomes:

  objects: [671] P4Parser name=p declid=1
    type: [30] Type_Parser name=p declid=0
      typeParameters: [5] TypeParameters
      applyParams: [26] ParameterList
        parameters: [10] Parameter name=check declid=0 direction=in
          type: [8] Type_Boolean
        parameters: [18] Parameter name=foo declid=1 direction=in
          type: [16] Type_Bits size=10 isSigned=0
        parameters: [23] Parameter name=matches declid=2 direction=out
          type: [21] Type_Boolean
    constructorParams: [153] ParameterList
    parserLocals: [490] Declaration_Variable name=val1 declid=12
      type: [2] Type_Bits size=32 isSigned=0
      initializer: [487] Cast
        type: [2] Type_Bits size=32 isSigned=0...
        expr: [486] Constant value=2 base=10
          type: [2] Type_Bits size=32 isSigned=0...
        destType: [2] Type_Bits size=32 isSigned=0...
    states: [696] ParserState name=start declid=4
      selectExpression: [699] SelectExpression
        type: [548] Type_State
        select: [701] ListExpression
          type: [516] Type_List
            components: [446] Type_Bits size=10 isSigned=0
            components: [510] Type_List
              components: [2] Type_Bits size=32 isSigned=0...
              components: [2] Type_Bits size=32 isSigned=0...

@asl asl requested review from ChrisDodd, fruffy and vlstill February 20, 2025 00:19
@asl asl added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 20, 2025
@asl
Consistently set & propagate child names
5eadb9f 
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
@asl asl force-pushed the stale-child-name branch from b2ecd72 to 5eadb9f Compare February 20, 2025 00:34
@ChrisDodd

ChrisDodd commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

So the child_name field was originally added to the context to aid in debugging -- when in gdb, you can look at the context and see this string to get a better idea where it was. Then it got used in the dump code to dump IR and that worked fine too. The main issue is that when dumping the children of an IR::Vector, they did not have names just indexes, so in Vector::visit_children, this field would not be set. This caused the dumper to output the elements of a Vector in sequence without a name: prefix.

This all worked fin until we introduced inline Vectors -- in this case the children were children of the Vectors parent ans so things got messed up.

I always wanted to improve the dumper to print the indexes for Vectors, but never came up with a good way of doing it without being too expensive -- we don't want to add overhead to all IR traversals (by, for example, creating dynamic strings to put in child_name) as this is something that is there to help debugging (the overhead of just storing an extra pointer to a static string for each node visited is not too bad).

Overall this looks good and is definitely an improvement. I'd still like to figure out a way to print indexes so you would end up with something like:

    type: [30] Type_Parser name=p declid=0
      typeParameters: [5] TypeParameters
      applyParams: [26] ParameterList
        parameters[0]: [10] Parameter name=check declid=0 direction=in
          type: [8] Type_Boolean
        parameters[1]: [18] Parameter name=foo declid=1 direction=in
          type: [16] Type_Bits size=10 isSigned=0
        parameters[2]: [23] Parameter name=matches declid=2 direction=out
          type: [21] Type_Boolean

without introducing extra overhead. My thought was to add some extra magic char to the static string that IRDumper could recognize and do something with, but I never got that far.

@asl

asl commented Feb 20, 2025

Copy link
Copy Markdown
Contributor Author

Overall this looks good and is definitely an improvement. I'd still like to figure out a way to print indexes so you would end up with something like:

Yeah, I wanted something like this as well, but thought that at least name is good enough :) Otherwise things were pretty misleading. Also note that the code in DoSynthesizeActions::preorder(IR::AssignmentStatement*) explicitly relies on child_name and wants it set (and therefore implicitly relies on stale name in the context :) )

@asl
Try to unbreak Tofino
9daf098 
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
@asl asl force-pushed the stale-child-name branch from 1ac66d7 to 9daf098 Compare February 24, 2025 20:29
@asl asl enabled auto-merge February 24, 2025 20:46
@asl asl added this pull request to the merge queue Feb 24, 2025
Merged via the queue into p4lang:main with commit d647430 Feb 24, 2025
@asl asl deleted the stale-child-name branch February 24, 2025 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrisDodd ChrisDodd ChrisDodd approved these changes

@vlstill vlstill Awaiting requested review from vlstill

@fruffy fruffy Awaiting requested review from fruffy

Assignees

No one assigned

Labels

core Topics concerning the core segments of the compiler (frontend, midend, parser)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asl @ChrisDodd