Remove ReferenceMap from top-level frontend passes

[asl]

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

• RemoveAllUnusedDeclarations uses lightweight implementation of used set that was factored out from ResolveReferences
• Added shortcut CheckShadow to execute ResolveReferences with shadow check enabled (and with "local" refmap)
• Made refmap and internal implementation details of Evaluator, Inliner and SimplifyDefUse

[fruffy]

I missed this in the other PRs but passing in nullptr doesn't seem right to me. Any way to make the constructor safe and just use two parameters? I also ran into this with ConstantFolding, which now has an ambiguous constructor.

[asl]

Yeah, this logic is pretty confusing, as depending whether refmap is passed or not the TypeChecking also runs ResolveReferences. And lots of passes rely on this.

But I think now we can add typemap-only constructor. And ensure that refmap is non-null on all other paths.

[vlstill]

Thanks for all these changes!

I'm not sure about the changes to evaluator in frontend and inlining. From the comment in inlining, it seems like the pass itself does not need to re-run evaluator, that it is just to update its state for future use. But then the evaluator is not used again if it is not passed into Inlining. Also in the frontend, the evaluator "result" is not used.

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...

p4c/frontends/p4/evaluator/evaluator.cpp

Lines 133 to 134 in 7009f96

	::P4::error(ErrorType::ERR_INVALID, "%1%: Cannot evaluate to a compile-time constant",
	arg->expression);

[asl]

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:

• It does its own traversal
• While it is an Inspector, it creates new nodes. Though, it does not attach them anywhere, they are used to build a hierarchy of CompileTimeValue / Block nodes similar to CFG.

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]

I think I addressed the majority of review comments.

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

[vlstill]

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]

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

[fruffy]

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]

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

[asl]

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