p4c icon indicating copy to clipboard operation
p4c copied to clipboard

Do not recompute write sets for equal child nodes belonging to a given `IR::Vector`

Open kfcripps opened this issue 1 year ago • 5 comments

built on top of https://github.com/p4lang/p4c/pull/4797

Title and discussion in https://github.com/p4lang/p4c/pull/4797 are self-explanatory.

This still does not fully fix the problem described in https://github.com/p4lang/p4c/pull/4797 (but it is yet another incremental improvement). ComputeWriteSet's callingContext is not properly updated during the IR traversal, so ProgramPoints are not correct in many places. This can be fixed by either:

  • Making sure that ComputeWriteSet's callingContext is properly updated and ProgramPoints created correctly everywhere, or
  • Replace all ProgramPoints with loc_t, so that updating the calling context is the responsibility of the Visitor overhead, rather than of ComputeWriteSet.

Neither of these are trivial, but the latter is probably the most invasive option.

kfcripps avatar Jul 16 '24 01:07 kfcripps

See https://github.com/p4lang/p4c/issues/4811

kfcripps avatar Jul 16 '24 02:07 kfcripps

Not particularly fond of this API. Seems built just to hide this particular bug.

mihaibudiu avatar Jul 19 '24 18:07 mihaibudiu

@mihaibudiu By API do you mean visit_unique_children()? I thought it might be useful for other passes that also do not want to traverse equal child nodes, but if you insist, I can make it local to ComputeWriteSet.

Also, this PR is not "hiding" any bug. It is one of several steps needed to fix the root problem that has been previously discussed.

kfcripps avatar Jul 19 '24 19:07 kfcripps

This is @ChrisDodd's territory, so he should decide the fate of this API

mihaibudiu avatar Jul 19 '24 21:07 mihaibudiu

(sorry I missed this PR when it first came around)

How do we get IR::Vectors with duplicate nodes in them? In all the cases where you're calling visit_unique_children it would seem that non-unique children are impossible (or at least should be a BUG if they ever occurred)?

For example, a BlockStatement like

a = a + 1;
a = a + 1;

that might be represented as the same Statement repeated twice, but the effect of each will be independent -- in particular, the use/defs of each will be different. This would imply that if this is a DAG with the same child appearing twice, we need to include the child_index in the loc_t to distinguish the two references.

ChrisDodd avatar Sep 03 '24 14:09 ChrisDodd