Skip to content

Shared: Fix bad join boundedPhiRankStep - #20576

Closed
MathiasVP wants to merge 1 commit into
github:mainfrom
MathiasVP:fix-bad-join-on-bounded-phi-rank-step
Closed

Shared: Fix bad join boundedPhiRankStep#20576
MathiasVP wants to merge 1 commit into
github:mainfrom
MathiasVP:fix-bad-join-on-bounded-phi-rank-step

Conversation

@MathiasVP

Copy link
Copy Markdown
Contributor

Before (when running cpp/invalid-pointer-deref on neovim):

Pipeline standard for RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015@6875a9w9 was evaluated in 8990 iterations totaling 30117ms (delta sizes total: 688791).
685940 ~1% {9} r1 = SCAN `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev_delta` OUTPUT In.0, In.1, In.2, In.3, In.4, In.5, In.6, _, In.7
685940 ~1% {8} | REWRITE WITH Tmp.7 := 1, Out.7 := (Tmp.7 + In.8) KEEPING 8
2143300387 ~1% {10} | JOIN WITH `RangeAnalysisImpl::ConstantStage::boundedPhiCandValidForEdge/9#17e05352#prev` ON FIRST 7 OUTPUT Lhs.0, Rhs.7, Rhs.8, Lhs.7, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6
582448 ~1% {8} | JOIN WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 4 OUTPUT Lhs.0, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, Lhs.9, Lhs.3
44172560 ~0% {9} r2 = SCAN `RangeAnalysisImpl::ConstantStage::boundedPhiCandValidForEdge/9#17e05352#prev_delta` OUTPUT In.0, In.7, In.8, In.1, In.2, In.3, In.4, In.5, In.6
44172560 ~0% {9} r3 = JOIN r2 WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 3 OUTPUT Lhs.0, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, _, Rhs.3
44172560 ~1% {9} | REWRITE WITH Tmp.7 := 1, Out.7 := (InOut.8 - Tmp.7)
4135 ~2% {8} | JOIN WITH `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev` ON FIRST 8 OUTPUT Lhs.0, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.8
44172560 ~0% {11} r4 = JOIN r2 WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 3 OUTPUT Lhs.0, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, Lhs.1, Lhs.2, Rhs.3, _
{10} | REWRITE WITH Tmp.10 := 1, TEST InOut.9 = Tmp.10 KEEPING 10
106321 ~3% {10} | SCAN OUTPUT In.0, In.7, In.8, _, In.1, In.2, In.3, In.4, In.5, In.6
106321 ~2% {10} | REWRITE WITH Out.3 := 1
106321 ~4% {8} | JOIN WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 4 OUTPUT Lhs.0, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, Lhs.9, _
106321 ~5% {8} | REWRITE WITH Out.7 := 1
692904 ~1% {8} r5 = r1 UNION r3 UNION r4
688879 ~1% {8} | AND NOT `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev`(FIRST 8)
return r5

After:

Pipeline standard for RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015@65a349wa was evaluated in 8990 iterations totaling 1016ms (delta sizes total: 688356).
44172868 ~3% {9} r1 = SCAN `RangeAnalysisImpl::ConstantStage::boundedPhiCandValidForEdge/9#17e05352#prev_delta` OUTPUT In.0, In.7, In.8, In.1, In.2, In.3, In.4, In.5, In.6
106399 ~5% {8} r2 = JOIN r1 WITH `_RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>:__#join_rhs` ON FIRST 3 OUTPUT Lhs.0, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, _
106399 ~5% {8} | REWRITE WITH Out.7 := 1
685940 ~1% {8} r3 = SCAN `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev_delta` OUTPUT In.0, In.7, In.1, In.2, In.3, In.4, In.5, In.6
659753 ~0% {10} | JOIN WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0_0_expr_123#join_rhs` ON FIRST 2 OUTPUT Lhs.0, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Rhs.2, Rhs.3, Rhs.4
581543 ~0% {8} | JOIN WITH `RangeAnalysisImpl::ConstantStage::boundedPhiCandValidForEdge/9#17e05352#prev` ON FIRST 9 OUTPUT Lhs.0, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.9
44172868 ~0% {9} r4 = JOIN r1 WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 3 OUTPUT Lhs.0, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, _, Rhs.3
44172868 ~1% {9} | REWRITE WITH Tmp.7 := 1, Out.7 := (InOut.8 - Tmp.7)
4135 ~2% {8} | JOIN WITH `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev` ON FIRST 8 OUTPUT Lhs.0, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.8
692077 ~1% {8} r5 = r2 UNION r3 UNION r4
688749 ~1% {8} | AND NOT `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev`(FIRST 8)
return r5

```
Pipeline standard for RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015@cc6f89w5 was evaluated in 8990 iterations totaling 85259ms (delta sizes total: 721932).
685940 ~0% {9} r1 = SCAN `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev_delta` OUTPUT In.0, In.1, In.2, In.3, In.4, In.5, In.6, _, In.7
685940 ~0% {8} | REWRITE WITH Tmp.7 := 1, Out.7 := (Tmp.7 + In.8) KEEPING 8
2160316026 ~1% {10} | JOIN WITH `RangeAnalysisImpl::ConstantStage::boundedPhiCandValidForEdge/9#17e05352#prev` ON FIRST 7 OUTPUT Lhs.0, Rhs.7, Rhs.8, Lhs.7, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6
613801 ~7% {8} | JOIN WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 4 OUTPUT Lhs.0, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, Lhs.9, Lhs.3
44172265 ~0% {9} r2 = SCAN `RangeAnalysisImpl::ConstantStage::boundedPhiCandValidForEdge/9#17e05352#prev_delta` OUTPUT In.0, In.7, In.8, In.1, In.2, In.3, In.4, In.5, In.6
44172265 ~0% {9} r3 = JOIN r2 WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 3 OUTPUT Lhs.0, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, _, Rhs.3
44172265 ~0% {9} | REWRITE WITH Tmp.7 := 1, Out.7 := (InOut.8 - Tmp.7)
4135 ~0% {8} | JOIN WITH `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev` ON FIRST 8 OUTPUT Lhs.0, Lhs.1, Lhs.2, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.8
44172265 ~0% {11} r4 = JOIN r2 WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 3 OUTPUT Lhs.0, Lhs.3, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, Lhs.1, Lhs.2, Rhs.3, _
{10} | REWRITE WITH Tmp.10 := 1, TEST InOut.9 = Tmp.10 KEEPING 10
119942 ~16% {10} | SCAN OUTPUT In.0, In.7, In.8, _, In.1, In.2, In.3, In.4, In.5, In.6
119942 ~17% {10} | REWRITE WITH Out.3 := 1
119942 ~24% {8} | JOIN WITH `RangeUtils::MakeUtils<SemanticLocation::SemLocation,RangeAnalysisImpl::Sem,FloatDelta::FloatDelta>::rankedPhiInput/4#d3c444f0` ON FIRST 4 OUTPUT Lhs.0, Lhs.4, Lhs.5, Lhs.6, Lhs.7, Lhs.8, Lhs.9, _
119942 ~22% {8} | REWRITE WITH Out.7 := 1
737878 ~9% {8} r5 = r1 UNION r3 UNION r4
721932 ~6% {8} | AND NOT `RangeAnalysisImpl::ConstantStage::boundedPhiRankStep/8#99a39015#prev`(FIRST 8)
return r5
```
@MathiasVP
MathiasVP requested review from aschackmull and Copilot and removed request for CopilotOctober 2, 2025 19:35
@MathiasVPMathiasVP changed the title Shared: Fix this bad join boundedPhiRankStepShared: Fix bad join boundedPhiRankStepOct 2, 2025
@MathiasVPMathiasVP added the no-change-note-required This PR does not need a change note label Oct 2, 2025
@aschackmull

Copy link
Copy Markdown
Contributor

Obviously the posted "After" order is much better, but this looks like a fix-by-coincidence to me. I don't see anything in the added binding pragmas that's in any way preventing the bad order. From the posted join-orders, the problem is quite clear to me, though, so let me see if I can write a better alternative.

@aschackmull

Copy link
Copy Markdown
Contributor

This fix here #20579 should be much better, I think.

@MathiasVP

Copy link
Copy Markdown
ContributorAuthor

I've merged your fix, Schack. Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@MathiasVP@aschackmull