Skip to content

Conversation

@aschackmull
Copy link
Contributor

Alternative to #18564. Hopefully this should be a much better join-order: This reorders the two readsVariable conjuncts pushing one into the offending predicate and pulling the other out. The result is variableEqualityCheckedInBlock(Variable checkedVar, IRBlock bb) which appears to have a good column correlation (i.e. small size), and it's simple enough to handle the inlined ensuresEq. This makes sense since its semantic interpretation also seems like a reasonable thing to materialise. The subsequent join is fast when it happens on both columns, which we ensure by adding the basic block as an additional column to readsVariable (it's functional so should be a safe addition).

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Jan 23, 2025
Copilot AI review requested due to automatic review settings January 23, 2025 13:25
@aschackmull aschackmull requested a review from a team as a code owner January 23, 2025 13:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@github-actions github-actions bot added the C++ label Jan 23, 2025
@aschackmull
Copy link
Contributor Author

aschackmull commented Jan 23, 2025

And here's the RA with tuple counts to compare.

[2025-01-23 14:27:49] Evaluated non-recursive predicate TaintedAllocationSize::variableEqualityCheckedInBlock/2#232e5d48@cbe71ch1 in 5ms (size: 453933).
Evaluated relational algebra for predicate TaintedAllocationSize::variableEqualityCheckedInBlock/2#232e5d48@cbe71ch1 with tuple counts:
        304632    ~0%    {2} r1 = JOIN `Operand::Operand.getDef/0#dispred#a70e8079_10#join_rhs` WITH `project#TaintedAllocationSize::readsVariable/3#4a43640b` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
          6291    ~4%    {3}    | JOIN WITH `project#IRGuards::Cached::compares_eq/6#511a0d6d_102#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Rhs.2
          6423    ~4%    {3}    | JOIN WITH `ValueNumberingInternal::tvalueNumber/1#f03b58f9_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1
        884468  ~264%    {2}    | JOIN WITH `IRGuards::IRGuardCondition.valueControls/2#eb6b9b19_021#join_rhs` ON FIRST 2 OUTPUT Lhs.2, Rhs.2
                         return r1

[2025-01-23 14:27:49] Evaluated non-recursive predicate TaintedAllocationSize::TaintedAllocationSizeConfig::isBarrier/1#6f365b45@d1b32bbo in 19ms (size: 240102).
Evaluated relational algebra for predicate TaintedAllocationSize::TaintedAllocationSizeConfig::isBarrier/1#6f365b45@d1b32bbo with tuple counts:
             32   ~2%    {1} r1 = JOIN Allocation::HeuristicAllocationFunction#class#57f08eba WITH `Function::Function.getAParameter/0#dispred#511fd682` ON FIRST 1 OUTPUT Rhs.1
             32   ~0%    {1}    | JOIN WITH `DataFlowUtil::Node.asParameter/0#dispred#81f7eba7_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1
                     
        1622515   ~0%    {2} r2 = JOIN DataFlowUtil::OperandNode#3e3b23f6_20#join_rhs WITH `Operand::Operand.getDef/0#dispred#a70e8079` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
         279667   ~0%    {3}    | JOIN WITH `TaintedAllocationSize::readsVariable/3#4a43640b` ON FIRST 1 OUTPUT Rhs.1, Rhs.2, Lhs.1
           7916   ~1%    {1}    | JOIN WITH `TaintedAllocationSize::variableEqualityCheckedInBlock/2#232e5d48` ON FIRST 2 OUTPUT Lhs.2
...

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM if DCA is happy.

@jketema jketema merged commit 4311553 into github:main Jan 23, 2025
14 checks passed
@aschackmull aschackmull deleted the cpp/join-order-fix-taintedallocationsize branch January 24, 2025 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants