Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kfcripps
Copy link
Contributor

@kfcripps kfcripps commented Jul 16, 2024

built on top of #4797

Title and discussion in #4797 are self-explanatory.

This still does not fully fix the problem described in #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
Copy link
Contributor Author

See #4811

@asl asl requested a review from ChrisDodd July 16, 2024 04:33
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Jul 16, 2024
@kfcripps kfcripps requested review from asl and mihaibudiu July 19, 2024 17:47
@kfcripps kfcripps marked this pull request as ready for review July 19, 2024 17:47
@kfcripps kfcripps requested a review from fruffy July 19, 2024 17:48
@mihaibudiu
Copy link
Contributor

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

@kfcripps
Copy link
Contributor Author

kfcripps commented Jul 19, 2024

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

@mihaibudiu
Copy link
Contributor

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

@fruffy fruffy requested review from ChrisDodd and removed request for ChrisDodd July 24, 2024 13:59
@ChrisDodd
Copy link
Contributor

ChrisDodd commented Sep 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants