C++: Refactor SSA usage in data flow.#18942
Merged
aschackmull merged 14 commits intogithub:mainfrom Mar 14, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
MathiasVP
reviewed
Mar 6, 2025
Contributor
MathiasVP
left a comment
MathiasVP
left a comment
There was a problem hiding this comment.
This looks great! If tests DCA comes back happy I'm perfectly happy merging this and leaving the small TODOs.
Update: I see CI is already finding some test discrepancies. Happy to take a look at those
Contributor
|
The result changes should be fixed by #18955 |
aschackmull
force-pushed
the
cpp/refactor-ssa
branch
from
bcf2cf8 to
409203c
Compare
Contributor
Author
|
Rebased and added two new commits. The first commit "C++: Fix spurious ExprNode fanout in DataFlowIntegration" should improve performance by using proper control-flow nodes instead of |
Contributor
|
And finally: The lost test result in the internal directory will be fixed by #19001 |
MathiasVP
force-pushed
the
cpp/refactor-ssa
branch
2 times, most recently
from
61f1f07 to
7600a2d
Compare
aschackmull
added
the
depends on internal PR
Before:
[2025-03-12 10:27:53] Evaluated non-recursive predicate SsaInternals::UseImpl.hasIndexInBlock/2#dispred#1e34a5af@e87543ui in 935ms (size: 8905695).
Evaluated relational algebra for predicate SsaInternals::UseImpl.hasIndexInBlock/2#dispred#1e34a5af@e87543ui with tuple counts:
{3} r1 = SsaInternals::DirectUseImpl#a58aae88 AND NOT `_ArithmeticOperation::PostfixCrementOperation#17623ada_Expr::UnaryOperation.getOperand/0#dispred#990__#antijoin_rhs`(FIRST 3)
8579337 ~4% {2} | SCAN OUTPUT In.1, In.0
8579337 ~0% {2} | JOIN WITH `Operand::Operand.getUse/0#dispred#427b49d0` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
8579337 ~0% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
48215 ~2% {2} r2 = SCAN SsaInternals::GlobalUse#9cd323b4 OUTPUT In.2, In.0
35467318 ~3% {2} | JOIN WITH `SSAConstruction::getInstructionEnclosingIRFunction/1#5443f355_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
48189 ~0% {2} r3 = JOIN r2 WITH Instruction::ReturnInstruction#28bfb7eb ON FIRST 1 OUTPUT Lhs.0, Lhs.1
12332 ~0% {2} r4 = JOIN r2 WITH Instruction::UnreachedInstruction#774c7a34 ON FIRST 1 OUTPUT Lhs.0, Lhs.1
60521 ~0% {2} r5 = r3 UNION r4
60521 ~2% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
39316 ~0% {2} r6 = JOIN SsaInternals::FinalParameterUse#c1f84700_10#join_rhs WITH `Parameter::Parameter.getFunction/0#dispred#803faca2` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
43821265 ~0% {2} | JOIN WITH `Instruction::Instruction.getEnclosingFunction/0#dispred#cb8ccc56_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
39194 ~0% {2} r7 = JOIN r6 WITH Instruction::ReturnInstruction#28bfb7eb ON FIRST 1 OUTPUT Lhs.0, Lhs.1
21255 ~2% {2} r8 = JOIN r6 WITH Instruction::UnreachedInstruction#774c7a34 ON FIRST 1 OUTPUT Lhs.0, Lhs.1
60449 ~0% {2} r9 = r7 UNION r8
60449 ~3% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
8784725 ~1% {5} r10 = JOIN `_SsaInternals::DirectUseImpl#a58aae88_SsaInternals::DirectUseImpl.getBase/0#dispred#4b8c43d0_SsaInte__#shared` WITH `SsaInternals::DirectUseImpl.getBase/0#dispred#4b8c43d0` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1, Lhs.2, Lhs.3
8784725 ~0% {5} | JOIN WITH `cached_SSAConstruction::getInstructionAst/1#d0d95b50` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3, Lhs.4
210435 ~4% {5} | JOIN WITH `Expr::UnaryOperation.getOperand/0#dispred#990de484#bf_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3, Lhs.4
205388 ~0% {4} | JOIN WITH ArithmeticOperation::PostfixCrementOperation#17623ada ON FIRST 1 OUTPUT Lhs.4, Lhs.3, Lhs.2, Lhs.1
205388 ~4% {3} | JOIN WITH `__IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs__ArithmeticOperation::PostfixCrementOperat__#join_rhs` ON FIRST 3 OUTPUT Rhs.4, Lhs.3, Rhs.3
205388 ~0% {3} | JOIN WITH `Operand::Operand.getUse/0#dispred#427b49d0` ON FIRST 1 OUTPUT Lhs.2, Rhs.1, Lhs.1
205388 ~1% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_021#join_rhs` ON FIRST 2 OUTPUT Lhs.2, Lhs.0, Rhs.2
8905695 ~0% {3} r11 = r1 UNION r5 UNION r9 UNION r10
return r11
After:
[2025-03-12 11:12:48] Evaluated non-recursive predicate SsaInternals::hasReturnPosition/3#02f7eab8@bc405c4l in 3ms (size: 49368).
Evaluated relational algebra for predicate SsaInternals::hasReturnPosition/3#02f7eab8@bc405c4l with tuple counts:
49368 ~3% {1} r1 = Instruction::ReturnInstruction#28bfb7eb UNION Instruction::UnreachedInstruction#774c7a34
49368 ~0% {2} | JOIN WITH `cached_SSAConstruction::getInstructionEnclosingIRFunction/1#5443f355` ON FIRST 1 OUTPUT Lhs.0, Rhs.1
49368 ~2% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
return r1
[2025-03-12 11:12:54] Evaluated non-recursive predicate SsaInternals::UseImpl.hasIndexInBlock/2#dispred#1e34a5af@6e30cduo in 549ms (size: 8905695).
Evaluated relational algebra for predicate SsaInternals::UseImpl.hasIndexInBlock/2#dispred#1e34a5af@6e30cduo with tuple counts:
48215 ~2% {2} r1 = SCAN SsaInternals::GlobalUse#9cd323b4 OUTPUT In.2, In.0
60521 ~2% {3} | JOIN WITH `SsaInternals::hasReturnPosition/3#02f7eab8` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
50725 ~0% {2} r2 = JOIN `IRFunctionBase::IRFunctionBase.getFunction/0#dispred#b024672e_10#join_rhs` WITH `Parameter::Parameter.getFunction/0#dispred#803faca2_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
39231 ~2% {2} | JOIN WITH SsaInternals::FinalParameterUse#c1f84700_10#join_rhs ON FIRST 1 OUTPUT Lhs.1, Rhs.1
60449 ~3% {3} | JOIN WITH `SsaInternals::hasReturnPosition/3#02f7eab8` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
{3} r3 = SsaInternals::DirectUseImpl#a58aae88 AND NOT `_ArithmeticOperation::PostfixCrementOperation#17623ada_Expr::UnaryOperation.getOperand/0#dispred#990__#antijoin_rhs`(FIRST 3)
8579337 ~1% {2} | SCAN OUTPUT In.1, In.0
8579337 ~0% {2} | JOIN WITH `Operand::Operand.getUse/0#dispred#427b49d0` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
8579337 ~1% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Rhs.2
8784725 ~0% {5} r4 = JOIN `_SsaInternals::DirectUseImpl#a58aae88_SsaInternals::DirectUseImpl.getBase/0#dispred#4b8c43d0_SsaInte__#shared` WITH `SsaInternals::DirectUseImpl.getBase/0#dispred#4b8c43d0` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1, Lhs.2, Lhs.3
8784725 ~0% {5} | JOIN WITH `cached_SSAConstruction::getInstructionAst/1#d0d95b50` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3, Lhs.4
210435 ~0% {5} | JOIN WITH `Expr::UnaryOperation.getOperand/0#dispred#990de484#bf_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2, Lhs.3, Lhs.4
205388 ~2% {4} | JOIN WITH ArithmeticOperation::PostfixCrementOperation#17623ada ON FIRST 1 OUTPUT Lhs.4, Lhs.3, Lhs.2, Lhs.1
205388 ~0% {3} | JOIN WITH `__IRBlock::Cached::getInstruction/2#627f9c61_201#join_rhs__ArithmeticOperation::PostfixCrementOperat__#join_rhs` ON FIRST 3 OUTPUT Rhs.4, Lhs.3, Rhs.3
205388 ~0% {3} | JOIN WITH `Operand::Operand.getUse/0#dispred#427b49d0` ON FIRST 1 OUTPUT Lhs.2, Rhs.1, Lhs.1
205388 ~0% {3} | JOIN WITH `IRBlock::Cached::getInstruction/2#627f9c61_021#join_rhs` ON FIRST 2 OUTPUT Lhs.2, Lhs.0, Rhs.2
8905695 ~0% {3} r5 = r1 UNION r2 UNION r3 UNION r4
return r5
aschackmull
force-pushed
the
cpp/refactor-ssa
branch
from
25a780d to
c230944
Compare
Contributor
Analysis of result changesneovim__neovim
vim__vim and openjdk-jdk
|
Contributor
|
Overall, the changes in code and results LGTM. It's a bit of a shame that we lose those two Since this is a fairly major change (and I'm not actually on the team that owns this code anymore!) I would appreciate it if someone from the C team would take a second look. I'll ping them on Slack |
jketema
approved these changes
Mar 14, 2025
Contributor
jketema
left a comment
jketema
left a comment
There was a problem hiding this comment.
Approve, per the internal discussion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit-by-commit review encouraged.
There are a bunch of minor commits, and then there's "C++: Use SSA data flow integration module", which is the main change that switches C++ from direct phi-read references to use the data flow integration module to construct use-use flow.
I've done a lot of local black-box testing of these changes and they generally looking very reasonable. There are minor changes on the order of 0.1% of the ssa-based data flow edges, but at this point I think further evaluation is best done via dca.