From e71ff35d7fb2d5418ccb9a171b92fe34977eeec4 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 22 Nov 2022 18:35:37 +0100 Subject: [PATCH 1/2] JIT: Avoid introducing data races in RBO RBO can forward sub relops into upcoming conditions. We avoid doing this if the local assigned is live out of the block, but there may still be uses between the def and the condition which could cause us to duplicate reads and introduce data races. The problem is the same as #62048, which was fixed to handle it for volatile operations. Now that we have an overapproximation of the number of SSA uses stored in their defs we can use this to handle the situation in general. This also means we no longer do the forward sub when there may be multiple uses, which makes sense from a costing perspective. Fix #78713 --- src/coreclr/jit/redundantbranchopts.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 55443f60a15464..a1881e1978846c 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1961,9 +1961,8 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) continue; } - // If the lcl defined here is live out, forward sub is problematic. - // We'll either create a redundant tree (as the original won't be dead) - // or lose the def (if we actually move the RHS tree). + // If the lcl defined here is live out we'll create a redundant tree + // (as the original won't be dead). // if (VarSetOps::IsMember(this, block->bbLiveOut, prevTreeLclDsc->lvVarIndex)) { @@ -1971,6 +1970,17 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) continue; } + // The local may have uses between the def and the JTRUE. Since we are + // using liberal VNs here we could introduce races by allowing + // duplicating the RHS of the assignment. + // + LclSsaVarDsc* ssaDefDsc = prevTreeLclDsc->GetPerSsaData(prevTreeLHS->AsLclVarCommon()->GetSsaNum()); + if (ssaDefDsc->GetNumUses() >= 2) + { + JITDUMP(" -- prev tree lcl V%02u has up to %d uses\n", prevTreeLcl, ssaDefDsc->GetNumUses()); + continue; + } + JITDUMP(" -- prev tree is viable candidate for relop fwd sub!\n"); candidateTree = prevTreeRHS; candidateStmt = prevStmt; From ba4b28bab81b83d93d95873b1818426b925b4bad Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 22 Nov 2022 19:01:29 +0100 Subject: [PATCH 2/2] Fix comment --- src/coreclr/jit/redundantbranchopts.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index a1881e1978846c..9fa10a7b83a139 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -1970,9 +1970,9 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) continue; } - // The local may have uses between the def and the JTRUE. Since we are - // using liberal VNs here we could introduce races by allowing - // duplicating the RHS of the assignment. + // The local may have uses between the def and the JTRUE. Duplicating + // reads is not allowed and in any case duplicating it in this case is + // probably not be worth it from a profitability perspective. // LclSsaVarDsc* ssaDefDsc = prevTreeLclDsc->GetPerSsaData(prevTreeLHS->AsLclVarCommon()->GetSsaNum()); if (ssaDefDsc->GetNumUses() >= 2)