Skip to content
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.

Sign up for GitHub

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

Disable OpHelperReg optimisation for Coroutines #6706

Closed
wants to merge 1 commit into from
Closed

Disable OpHelperReg optimisation for Coroutines #6706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions lib/Backend/LinearScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3339,7 +3339,7 @@ LinearScan::KillImplicitRegs(IR::Instr *instr)
return;
}

if (instr->m_opcode == Js::OpCode::Yield)
if (instr->m_opcode == Js::OpCode::GeneratorBailInLabel)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The OpCode Yield is removed earlier in the jitting process and replaced with a number of operations - the one we're looking for here is the GeneratorBailInLabel.

{
this->bailIn.SpillRegsForBailIn();
return;
Expand Down Expand Up @@ -5041,6 +5041,7 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
// - We don't need to restore argObjSyms because StackArgs is currently not enabled
// Commented out here in case we do want to enable it in the future:
// this->InsertRestoreSymbols(bailOutInfo->capturedValues->argObjSyms, insertionPoint, saveInitializedReg);
Assert(!this->func->IsStackArgsEnabled());
//
// - We move all argout symbols right before the call so we don't need to restore argouts either

Expand All @@ -5050,13 +5051,7 @@ IR::Instr* LinearScan::GeneratorBailIn::GenerateBailIn(IR::GeneratorBailInInstr*
bailInInstr->capturedValues
);

this->InsertRestoreSymbols(
*bailOutInfo->byteCodeUpwardExposedUsed,
bailInInstr->upwardExposedUses,
bailInInstr->capturedValues,
insertionPoint
);
Assert(!this->func->IsStackArgsEnabled());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This Assert had gone adrift from the comment that explained it - therefore moved upwards.

this->InsertRestoreSymbols(insertionPoint);

#ifdef ENABLE_DEBUG_CONFIG_OPTIONS
if (PHASE_TRACE(Js::Phase::BailInPhase, this->func))
Expand Down Expand Up @@ -5201,12 +5196,7 @@ void LinearScan::GeneratorBailIn::BuildBailInSymbolList(
AssertOrFailFastMsg(unrestorableSymbols.IsEmpty(), "There are unrestorable backend-only symbols across yield points");
}

void LinearScan::GeneratorBailIn::InsertRestoreSymbols(
const BVSparse<JitArenaAllocator>& byteCodeUpwardExposedUses,
const BVSparse<JitArenaAllocator>& upwardExposedUses,
const CapturedValues& capturedValues,
Comment on lines -5205 to -5207
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These parameters weren't used

BailInInsertionPoint& insertionPoint
)
void LinearScan::GeneratorBailIn::InsertRestoreSymbols(BailInInsertionPoint& insertionPoint)
{
FOREACH_SLISTBASE_ENTRY(BailInSymbol, bailInSymbol, this->bailInSymbols)
{
Expand Down
8 changes: 2 additions & 6 deletions lib/Backend/LinearScan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//-------------------------------------------------------------------------------------------------------
// Copyright (C) Microsoft. All rights reserved.
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
//-------------------------------------------------------------------------------------------------------
#pragma once
Expand Down Expand Up @@ -296,12 +297,7 @@ class LinearScan
IR::SymOpnd* CreateGeneratorObjectOpnd() const;

// Insert instructions to restore symbols in the `bailInSymbols` list
void InsertRestoreSymbols(
const BVSparse<JitArenaAllocator>& bytecodeUpwardExposedUses,
const BVSparse<JitArenaAllocator>& upwardExposedUses,
const CapturedValues& capturedValues,
BailInInsertionPoint& insertionPoint
);
void InsertRestoreSymbols(BailInInsertionPoint& insertionPoint);

// Fill `bailInSymbols` list with all of the symbols that need to be restored
void BuildBailInSymbolList(
Expand Down
15 changes: 13 additions & 2 deletions lib/Backend/SccLiveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//-------------------------------------------------------------------------------------------------------
// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
//-------------------------------------------------------------------------------------------------------

Expand Down Expand Up @@ -260,9 +261,19 @@ SCCLiveness::Build()
{
this->EndOpHelper(labelInstr);
}
if (labelInstr->isOpHelper && !PHASE_OFF(Js::OpHelperRegOptPhase, this->func))
if (labelInstr->isOpHelper)
{
this->lastOpHelperLabel = labelInstr;
if (!PHASE_OFF(Js::OpHelperRegOptPhase, this->func) &&
!this->func->GetTopFunc()->GetJITFunctionBody()->IsCoroutine())
{
this->lastOpHelperLabel = labelInstr;
}
#ifdef DBG
else
{
labelInstr->AsLabelInstr()->m_noHelperAssert = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's an assertion to check that helper blocks are being set correctly - with this optimisation disabled it would fire incorrectly, setting this debug only property disables it.

}
#endif
}
}
else if (instr->IsBranchInstr() && !instr->AsBranchInstr()->IsMultiBranch())
Expand Down
16 changes: 16 additions & 0 deletions test/es6/generator-jit-bugs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,5 +152,21 @@ check(gf8().next().value.v8, 1.1);
check(gf8().next().value.v8, 1.1);
check(gf8().next().value.v8, 1.1);

// Test 9 - Invalid OpHelperBlockReg spill
title("Inner function access after for...in loop containing yield that is not hit")
{ // Note this bug only occurred when generator was declared inside an outer scope block
let i = 0;
function* gf9() {
function test() {}
for (var unused in []) {
yield undefined;
}
if(++i < 4)
gf9().next()
test + 1;
}
}
check(gf9().next().done, true);


print("pass");