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

Remove reference map from another set of midend passes #4939

Merged
merged 14 commits into from
Oct 4, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented Oct 3, 2024

This includes less obvious cases. Plus some additional cleanup.

asl added 10 commits October 2, 2024 18:50
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
@asl asl added core Topics concerning the core segments of the compiler (frontend, midend, parser) run-validation Use this tag to trigger a Validation CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. breaking-change This change may break assumptions of compiler back ends. labels Oct 3, 2024
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
…ce map

Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
…uses relies on fresh refmap

Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
@asl
Copy link
Contributor Author

asl commented Oct 3, 2024

Two issues were uncovered so far:

  • DPDK backend has pass (DismantleMuxExpression) that relies on fresh refmap, but does not rebuild it. The code before PR is already problematic and worked by coincidence :)
  • BMV2 PSA midend uses custom local copy propagation policy that relies on fresh external reference map (like "global variable"), so we really need to have refmap current there before local_copyprop.

Overall I think eventually we'd need passes to maintain their own copy of reference map if they really need it; we should not have global refmap object passed everywhere as maintaining its freshness is non-trivial and error-prone.

test/gtest/constant_folding.cpp Outdated Show resolved Hide resolved
test/gtest/frontend_test.cpp Outdated Show resolved Hide resolved
explicit DoConstantFolding(const TypeMap *typeMap, bool warnings = true,
ConstantFoldingPolicy *policy = nullptr)
: DoConstantFolding(this, typeMap, warnings, policy) {}

DoConstantFolding() : DoConstantFolding(nullptr, nullptr) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be DoConstantFolding() : DoConstantFolding(this, nullptr) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one very subtle but important difference. We do use refmap in absence of typemap during constfolding of path expressions. However, this requires more or less complete IR. This is not the case for P14-to-16 conversion, where constfolding is done in the middle of conversion and therefore we just crash not being able to resolve anything.

On the other hand, it seems we're missing some constfolding opportunities if we'd require known types along with resolution contex.

Things are a bit messy, yes, I am planning to untangle this eventually.

Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
@asl asl added this pull request to the merge queue Oct 3, 2024
Merged via the queue into p4lang:main with commit 9132140 Oct 4, 2024
18 checks passed
@asl asl deleted the midend-refmap2 branch October 4, 2024 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This change may break assumptions of compiler back ends. core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-validation Use this tag to trigger a Validation CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants