p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
683 stars 446 forks source link

Remove ReferenceMap from top-level frontend passes #4947

Closed asl closed 1 month ago

asl commented 1 month ago

Finally, this removes ReferenceMap from top-level frontend passes:

asl commented 1 month ago

As Evaluator is an inspector, presumably running it does not change anything, do we even need it in Frontend? Or is it because of errors it can produce? There seems to be just one...

Yeah, I was confused as well. And applied the common logic: if something looks incomplete, then probably there is a downstream code that uses / changes this. So, I tried to keep the existing logic as much as possible.

The Evaluator itself is a bit special:

The inliner (DiscoverInlining specifically) does depend on the evaluator as it seems to traverse Block's from the top level block, not the whole program. Given that Inliner runs in PassRepeated we'd need to update that CFG re-running evaluator.

I think we'd can just remove it from Frontend, yes.

asl commented 1 month ago

I think I addressed the majority of review comments.

@fruffy I'm going to do TypeChecking changes in a separate PR.

vlstill commented 1 month ago

The inliner (DiscoverInlining specifically) does depend on the evaluator as it seems to traverse Block's from the top level block, not the whole program. Given that Inliner runs in PassRepeated we'd need to update that CFG re-running evaluator.

Oh, I missed that inliner runs as PassRepeated, sorry.

asl commented 1 month ago

Looks like both bazel and fedora are unhealthy. @grg @fruffy do you know what's going on?

fruffy commented 1 month ago

Looks like both bazel and fedora are unhealthy. @grg @fruffy do you know what's going on?

It's possible that one of the new includes is breaking the Bazel build related to alloc_trace. For Fedora, unclear how that error happens. Hope it is spurious.

fruffy commented 1 month ago

@smolkaj Any idea whether the Bazel failures are spurious or require a change?

asl commented 1 month ago

@fruffy @smolkaj Bazel seems to be recovered. Only Fedora left...