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

improvement(semantic/cfg): rework basic blocks. #3381

Merged

Conversation

rzvxa
Copy link
Collaborator

@rzvxa rzvxa commented May 21, 2024

I've replaced the BasicBlockElement with an Instruction type which would keep both the instruction kind and its associated AstNodeId.
I also removed the register scheme in the control flow in favor of a simpler approach using explicit enums.

#3381 (comment)

Copy link

graphite-app bot commented May 21, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the A-semantic Area - Semantic label May 21, 2024
Copy link

codspeed-hq bot commented May 21, 2024

CodSpeed Performance Report

Merging #3381 will degrade performances by 3.89%

Comparing 05-22-improvement_semantic_cfg_rework_basic_blocks (209a99d) with main (646b993)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 20 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 05-22-improvement_semantic_cfg_rework_basic_blocks Change
transformer[RadixUIAdoptionSection.jsx] 286.1 µs 267.7 µs +6.9%
transformer[pdf.mjs] 61 ms 63.5 ms -3.89%

@Boshen Boshen force-pushed the 05-22-refactor_semantic_cfg_alias_petgraph_s_nodeindex_as_basicblockid_ branch from e0649aa to 78e6326 Compare May 22, 2024 03:09
@Boshen Boshen changed the base branch from 05-22-refactor_semantic_cfg_alias_petgraph_s_nodeindex_as_basicblockid_ to main May 22, 2024 03:13
@rzvxa rzvxa force-pushed the 05-22-improvement_semantic_cfg_rework_basic_blocks branch from 688a00b to 69aca4e Compare May 22, 2024 19:38
@mysteryven
Copy link
Member

mysteryven commented May 23, 2024

Look forward to this series of works. Looks like we can get AstNode directly by visiting CFG after this, Will it provide more features?

@rzvxa
Copy link
Collaborator Author

rzvxa commented May 23, 2024

Look forward to this series of works. Looks like we can get AstNode directly by visiting CFG after this, Will it provide more features?

This PR will introduce Instruction to replace BasicBlockElement, They're really similar but since Instruction can be any statement, We would be able to check if a given statement in a BasicBlock comes exactly before or after a jump/return; On top of this, we can build better inference and a more ergonomic API that might mimic the path segment stacks more closely.

So there are 3 major points that I'm trying to achieve. Some will happen in this PR and some I will do shortly.

  1. Associate a CFG instruction with its origin node if one exists(we have dummy blocks with no node so it is optional)
  2. Be more specific on instruction types and express a jump as an instruction(in addition to its edge which already exists). This helps us infer whether a jump happens before or after another statement, It might be unnecessary but it's too soon for me to say for sure. Could be possible to provide this information with a better API instead of adding redundant information?
  3. Keep track of the unwinding path and provide a way to follow the error path up the stack.

With all that said, I'm pretty much-adding features and refactoring stuff as I find shortcomings while implementing something. So no cleaver querying and/or super specific CFG visitors are going to come out of this; Unless somebody reports an actual use case for it.

@github-actions github-actions bot added the A-linter Area - Linter label May 24, 2024
@rzvxa rzvxa force-pushed the 05-22-improvement_semantic_cfg_rework_basic_blocks branch from ed52f80 to 6a84382 Compare May 25, 2024 12:25
@rzvxa rzvxa changed the base branch from main to 05-24-fix_linter_memorize_visited_block_id_in_neighbors_filtered_by_edge_weight_ May 25, 2024 12:25
@rzvxa rzvxa force-pushed the 05-22-improvement_semantic_cfg_rework_basic_blocks branch from 89de86d to 36e7010 Compare May 25, 2024 12:44
@mysteryven mysteryven force-pushed the 05-24-fix_linter_memorize_visited_block_id_in_neighbors_filtered_by_edge_weight_ branch from 3e830d1 to ee57dab Compare May 26, 2024 07:14
@Boshen Boshen force-pushed the 05-24-fix_linter_memorize_visited_block_id_in_neighbors_filtered_by_edge_weight_ branch 3 times, most recently from c232751 to 5e06298 Compare May 26, 2024 08:11
@Boshen Boshen changed the base branch from 05-24-fix_linter_memorize_visited_block_id_in_neighbors_filtered_by_edge_weight_ to main May 26, 2024 08:15
@rzvxa rzvxa force-pushed the 05-22-improvement_semantic_cfg_rework_basic_blocks branch 6 times, most recently from f48a6c6 to 2fa52e3 Compare May 28, 2024 01:21
@rzvxa rzvxa force-pushed the 05-22-improvement_semantic_cfg_rework_basic_blocks branch from 2fa52e3 to 5888b41 Compare May 28, 2024 12:40
@rzvxa
Copy link
Collaborator Author

rzvxa commented May 30, 2024

Reviewing this stack is becoming more cumbersome by the minute, I'll mark some parts of it that I'm sure are correct for review.
Is there a kind soul out there who would put up with these changes and give it a spin?

I'll write a summary for each PR I mark as "ready".

Copy link

graphite-app bot commented Jun 6, 2024

Merge activity

I've replaced the `BasicBlockElement` with an `Instruction` type which would keep both the instruction kind and its associated AstNodeId.
I also removed the register scheme in the control flow in favor of a simpler approach using explicit enums.

#3381 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants