[P4fmt]: attaching comments to IR Nodes

[snapdgn]

No description provided.

[snapdgn]

This takes care of attachment of comments to basic parent Node types(Typedef, Type_Name, control blocks, structs etc).
The Node List is not exhaustive and underdeveloped, and support for further nodes will be added gradually.
It currently attempts to store the comments related to each node in two lists, 'before' (holds comments that are directly above the nodes) and after(contains comments that are on the same line as the nodes).
Plan is to hold all the comments related to a node and attach them at all at appropriate positions based on column information.
The comment list will store a list of code points, with each entry containing a pair of the SourcePosition and the corresponding comment.

[fruffy]

What's the criterion for a supported node here? Otherwise I would use IR::Statement, IR::Type_Declaration, or IR::StatOrDecl here?

[qobilidop]

My understanding of the algorithm presented in the blog post (the Bazel approach) is that the preorder traversal will visit all AST nodes, and it can pick the right AST node to attach comments to automatically. The first node with a comment right before it will get that comment attached to it, and that comment will be removed from the list of comments to be attached, so that any child AST node on the same line won't get the same comment attached again.

This algorithm can guarantee a pretty reasonable attachment of line comments. And a similar algorithm can be applied in a postorder traversal to attach the inline trailing/suffix comments.

@fruffy I don't have enough knowledge on this, but is there a more convenient way in P4C to express that I want to do the same thing for all AST nodes regardless of its type? Maybe as you mentioned in another comment, adding IR::Node to a preorder is the way to go?

[vlstill]

I don't think enumerating all/most node types is a good idea (the fact the actual implementation is the same is a good hint for that!). It defeats the purpose of having the nodes in a class hierarchy and it would make this very fragile to IR node addition. From what @qobilidop described as the high-level algorithm, it might even work to have just a single preorder(Node *) and postoder(Node *). Comment attachment does not really seem to dependent on whether the node is e.g. an addition or subtraction, it even does not seem depend on whether it is e.g. an expression or a type definition. Even in cases of comments inside statements like x = foo(/* block size */ a + /* block count */ b); // get ... one would probably get a good enough comment attachment (the only question is really if the trailing comment belongs to the assignment or the call (where postorder would likely place it. But I guess that can't be really said in general and either will be good enough.

[vlstill]

From the point of the compiler in general, I think there should not the the mutable in the SourceInfo and I am concern if this could impact compiler performance.

Other than that, I don't think naming all the node types is a good approach.

[vlstill]

Another point -- this make the SourceInfo, which is ubiquitous in the compiler, quite a bit larger (by 6 pointers with the common impl of vector). It would be good to have some benchmarks to see this does not unduly show down the compiler (not p4fmt backend) or increase its memory consumption too much.

[asl]

If we'd go this way, I'd strongly suggest using small vectors that could hold up to 1 entry inline. But overall I concur that enlarging every IR node by this amount should be carefully benchmarked. We do create lots of nodes. There is huge malloc traffic everywhere. And while another 48 bytes might look small, it is an overhead paid on every node. Even if there are zero comments.

Also, comments are quite rare and here the price is paid by every node. Maybe the better solution is to use the technique similar to TrailingObjects in LLVM / clang.

[snapdgn]

Instead of storing those prefix and suffix comments as a part of SourceInfo, I was thinking how about storing that extra object in a hashmap, Hashmap<node-id, extra-comment-object>. This would avoid the unnecessary overhead to the compiler.
Would this be a good and straightforward solution for the time being?

[asl]

What is your plan to update this side map on IR change? Overall, side long-living maps should be discouraged as they could easily go stale

[qobilidop]

Just want to add some more context. We were thinking about using this side map only in p4fmt. With this constraint, there probably isn't any IR change expected I guess.

@snapdgn has also looked into using TrailingObjects. I agree that would be a better solution. My suggestion is to go with a simple solution for now, to unblock further p4fmt experimentation, as long as the solution doesn't influence the rest of P4C. Then switching to the TrailingObjects technique could be left as a future optimization.

@asl What do you think?

[asl]

if this side map is local, then certainly we're fine.

[vlstill]

I don't think enumerating all/most node types is a good idea (the fact the actual implementation is the same is a good hint for that!). It defeats the purpose of having the nodes in a class hierarchy and it would make this very fragile to IR node addition. From what @qobilidop described as the high-level algorithm, it might even work to have just a single preorder(Node *) and postoder(Node *). Comment attachment does not really seem to dependent on whether the node is e.g. an addition or subtraction, it even does not seem depend on whether it is e.g. an expression or a type definition. Even in cases of comments inside statements like x = foo(/* block size */ a + /* block count */ b); // get ... one would probably get a good enough comment attachment (the only question is really if the trailing comment belongs to the assignment or the call (where postorder would likely place it. But I guess that can't be really said in general and either will be good enough.

[snapdgn]

Comments are stored in a side map for now, as discussed.

[fruffy]

Quick round of comments.

It would be good to have a reference output here. We can use the checker that you wrote. This way we can see the current behavior.

[qobilidop]

There are merge conflicts with the main branch. Please rebase/merge and resolve them.

[snapdgn]

Would be grateful for any updated feedback on this.

[snapdgn]

Almost all of the suggestions and comments have been addressed.

I'm planning on taking care of optimizations in a separate PR. (Sorting comments for easy lookup, etc).

I would appreciate a final review to confirm if this is ready for merging.

[qobilidop]

@asl @vlstill Any remaining concerns? I'll try to merge this if no new issues are raised.

[asl]

what is the purpose of fopen here? Just for "no such file" diagnostics as file is unused below? What does parseProgramSources reports in case no input file?

[snapdgn]

Switched to std::filesystem::exists , if that helps?

[asl]

Do you really need this? What does parseProgramSources repors in case non-existent input file?

[snapdgn]

I believe this is necessary as parseProgramSources doesn't report anything if there's no input file, it just fails to parse and returns null. There are no other checks to validate this.

[asl]

See comments. There are no tests as well.

[asl]

@asl @vlstill Any remaining concerns? I'll try to merge this if no new issues are raised.

@qobilidop See my comments on concerns

[snapdgn]

See comments. There are no tests as well.

The tests are intended to be included as part of this PR.