Skip to content

C++: Fix two more dataflow-related joins #18307

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

Merged
merged 5 commits into from
Dec 17, 2024

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Dec 17, 2024

The join order fix in #18233 turned out to be slightly incomplete. This can actually be seen in the 'after' tuple counts, but I decided to ignore it since I couldn't find a repository where it was too bad 🙈

Well, jokes on me, because there was actually a repository where this happened (thanks for flagging this up to me, @jketema):

[2024-12-17 14:53:19] Evaluated non-recursive predicate DataFlowUtil::BarrierGuard<UnboundedWrite::lessThanOrEqual>::getABarrierNode/0#fcc0c149@37a2adb9 in 266575ms (evaluation was halted due to CancellationException).
Evaluated relational algebra for predicate DataFlowUtil::BarrierGuard<UnboundedWrite::lessThanOrEqual>::getABarrierNode/0#fcc0c149@37a2adb9 with tuple counts:
    2067749       ~0%    {2} r1 = JOIN DataFlowUtil::OperandNode#3e3b23f6_20#join_rhs WITH `Operand::Operand.getDef/0#dispred#a70e8079` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
    1269345       ~2%    {2}    | JOIN WITH `Instruction::Instruction.getConvertedResultExpression/0#dispred#f9cfafab` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
     481892       ~2%    {2}    | JOIN WITH `SsaInternals::DefinitionExt.getARead/0#0429944e_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
   29735623      ~70%    {3}    | JOIN WITH `UnboundedWrite::lessThanOrEqual/3#4e4dda57_102#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Rhs.2, Lhs.1
    1471195   ~11490%    {2}    | JOIN WITH `DataFlowUtil::guardControlsPhiInput/5#50adc50d` ON FIRST 3 OUTPUT Rhs.4, Rhs.3
    1440561   ~11292%    {1}    | JOIN WITH DataFlowUtil::TSsaPhiInputNode#d9cd78c6 ON FIRST 2 OUTPUT Rhs.2
                     
    3299500     ~705%    {3} r2 = JOIN `UnboundedWrite::lessThanOrEqual/3#4e4dda57_102#join_rhs` WITH `Instruction::Instruction.getConvertedResultExpression/0#dispred#f9cfafab_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
    3294500    ~3478%    {3}    | JOIN WITH `ValueNumberingInternal::tvalueNumber/1#f03b58f9` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
20015752673    ~3597%    {3}    | JOIN WITH `ValueNumberingInternal::tvalueNumber/1#f03b58f9_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
20014034357    ~3405%    {3}    | JOIN WITH `Instruction::Instruction.getConvertedResultExpression/0#dispred#f9cfafab` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
20013966490    ~4007%    {3}    | JOIN WITH `project#ExprNodes::ExprNode.getConvertedExpr/1#dispred#04e26388_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2
20013961855    ~3810%    {4}    | JOIN WITH `project#DataFlowUtil::Node.hasIndexInBlock/2#4ca72ac1` ON FIRST 1 OUTPUT Lhs.1, Rhs.1, Lhs.2, Lhs.0
      22500    ~2307%    {1}    | JOIN WITH `IRGuards::IRGuardCondition.controls/2#dispred#a7389eda` ON FIRST 3 OUTPUT Lhs.3
                     
    1463061  ~156413%    {1} r3 = r1 UNION r2
                         return r3

(I stopped the run before it was finished)

This PR fixes that join by ordering the IRGuards::IRGuardCondition.controls join before the second join with ValueNumberingInternal::tvalueNumber which is clearly better:

Evaluated relational algebra for predicate DataFlowUtil::BarrierGuard<UnboundedWrite::lessThanOrEqual>::getABarrierNode/0#fcc0c149@32b236qn with tuple counts:
    2067749      ~1%    {2} r1 = JOIN DataFlowUtil::OperandNode#3e3b23f6_20#join_rhs WITH `Operand::Operand.getDef/0#dispred#a70e8079` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
    1269345      ~0%    {2}    | JOIN WITH `Instruction::Instruction.getConvertedResultExpression/0#dispred#f9cfafab` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
     481892      ~1%    {2}    | JOIN WITH `SsaInternals::DefinitionExt.getARead/0#0429944e_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
   29735623     ~74%    {3}    | JOIN WITH `UnboundedWrite::lessThanOrEqual/3#4e4dda57_102#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Rhs.2, Lhs.1
    1471329  ~11826%    {2}    | JOIN WITH `DataFlowUtil::guardControlsPhiInput/5#50adc50d` ON FIRST 3 OUTPUT Rhs.4, Rhs.3
    1441442  ~11783%    {1}    | JOIN WITH DataFlowUtil::TSsaPhiInputNode#d9cd78c6 ON FIRST 2 OUTPUT Rhs.2
                    
   59515338      ~0%    {3} r2 = SCAN `UnboundedWrite::lessThanOrEqual/3#4e4dda57` OUTPUT In.0, In.2, In.1
    9908538      ~0%    {2}    | JOIN WITH `IRGuards::IRGuardCondition.controls/2#dispred#a7389eda_021#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Lhs.2
  364723786      ~0%    {2}    | JOIN WITH `project#DataFlowUtil::Node.hasIndexInBlock/2#4ca72ac1_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
  362546872      ~1%    {2}    | JOIN WITH `Instruction::Instruction.getConvertedResultExpression/0#dispred#f9cfafab_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1
  362546213   ~4197%    {2}    | JOIN WITH `ValueNumberingInternal::tvalueNumber/1#f03b58f9` ON FIRST 1 OUTPUT Lhs.1, Rhs.1
    1654638     ~23%    {3}    | JOIN WITH `project#ExprNodes::ExprNode.getConvertedExpr/1#dispred#04e26388` ON FIRST 1 OUTPUT Rhs.1, Lhs.0, Lhs.1
    1290054      ~6%    {3}    | JOIN WITH `Instruction::Instruction.getConvertedResultExpression/0#dispred#f9cfafab_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.2, Lhs.1
      24183      ~4%    {1}    | JOIN WITH `ValueNumberingInternal::tvalueNumber/1#f03b58f9` ON FIRST 2 OUTPUT Lhs.2
                    
    1465625   ~4008%    {1} r3 = r1 UNION r2
                        return r3

Ideally, I would have liked to order the joins with ExprNode.getConvertedExpr before the join with Node.hasIndexInBlock, but I couldn't quite make that work out. In any case, this join order is still much better than before.

The same project also showed an unfortunate materialization of ensuresEq:

[2024-12-17 15:04:03] Evaluated non-recursive predicate DataFlowPrivate::ensuresEq/5#44fb628b@559ac42h in 17131ms (size: 181724202).
Evaluated relational algebra for predicate DataFlowPrivate::ensuresEq/5#44fb628b@559ac42h with tuple counts:
[2024-12-17 15:03:48] Evaluated non-recursive predicate DataFlowPrivate::ensuresLt/5#598de9e7_10234#join_rhs@1c1067or in 1817ms (size: 12553976).
Evaluated relational algebra for predicate DataFlowPrivate::ensuresLt/5#598de9e7_10234#join_rhs@1c1067or with tuple counts:
  12553976  ~2%    {5} r1 = SCAN `DataFlowPrivate::ensuresLt/5#598de9e7` OUTPUT In.1, In.0, In.2, In.3, In.4
                   return r1
...
[2024-12-17 15:04:03] Evaluated non-recursive predicate DataFlowPrivate::ensuresEq/5#44fb628b@559ac42h in 17131ms (size: 181724202).
Evaluated relational algebra for predicate DataFlowPrivate::ensuresEq/5#44fb628b@559ac42h with tuple counts:
  181734680  ~4%    {5} r1 = JOIN `_IRGuards::IRGuardCondition.valueControls/2#eb6b9b19_ValueNumberingInternal::tvalueNumber/1#f03b58f9#shared` WITH `IRGuards::Cached::compares_eq/6#511a0d6d_051234#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Rhs.3, Rhs.4, Lhs.2, Rhs.5
                    return r1

Both of these have been removed in f351558.

@Copilot Copilot AI review requested due to automatic review settings December 17, 2024 15:42
@MathiasVP MathiasVP requested a review from a team as a code owner December 17, 2024 15:42
Copy link
Contributor

@Copilot 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 encountered an error and was unable to review this pull request. You can try again, by re-requesting a review.

@github-actions github-actions bot added the C++ label Dec 17, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Dec 17, 2024
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

@MathiasVP MathiasVP merged commit dfb3483 into github:main Dec 17, 2024
13 of 14 checks passed
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