Skip to content

Commit

Permalink
[1.10>master] [MERGE #5504 @sigatrev] Code Quality: use legalizePostR…
Browse files Browse the repository at this point in the history 
…egAlloc flag on Func instead of passing bool to Legalizers

Merge pull request #5504 from sigatrev:users/magardn/legalize

ARM and ARM64 legalizers behave differently before and after register allocation. We've had bugs in the past where, mainly through static helper functions similar to Lowerer::InsertMove where we've had the flag wrong. This commit adds a flag on Func to tell the legalizer if it should behave as if register allocation has happened or not, and removes the need to pass the flags. The flag was not used on x86 and x64

There were a couple of places where the flag passed did not agree with whether register allocation has finished, in particular LinearScan::InsertLea (passed the flag true) and ARM and ARM64 LowererMD::ChangeToAssign (always passed false). I've added an RAII auto-restore helper to change the flag on the Func while those calls are in progress . While not ideal, it does have the benefit of being mroe expressive.

I'm using a new flag instead of the existing (dbg only) isPostRegAlloc flag because the flag can be temporarily overriden.
  • Loading branch information
@sigatrev
sigatrev committed Jul 24, 2018
2 parents f8154dd + 9796560 commit 4964fc4
Show file tree
Hide file tree
Showing 16 changed files with 332 additions and 318 deletions.
29 changes: 7 additions & 22 deletions lib/Backend/Func.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
hasInstrNumber(false),
maintainByteCodeOffset(true),
frameSize(0),
topFunc(parentFunc ? parentFunc->topFunc : this),
parentFunc(parentFunc),
argObjSyms(nullptr),
m_nonTempLocalVars(nullptr),
Expand Down Expand Up @@ -109,6 +110,7 @@ Func::Func(JitArenaAllocator *alloc, JITTimeWorkItem * workItem,
isTJLoopBody(false),
m_nativeCodeDataSym(nullptr),
isFlowGraphValid(false),
legalizePostRegAlloc(false),
#if DBG
m_callSiteCount(0),
#endif
Expand Down Expand Up @@ -1314,6 +1316,11 @@ Func::EndPhase(Js::Phase tag, bool dump)
}
#endif

if (tag == Js::RegAllocPhase)
{
this->legalizePostRegAlloc = true;
}

#if DBG
if (tag == Js::LowererPhase)
{
Expand Down Expand Up @@ -1352,28 +1359,6 @@ Func::EndPhase(Js::Phase tag, bool dump)
#endif
}

Func const *
Func::GetTopFunc() const
{
Func const * func = this;
while (!func->IsTopFunc())
{
func = func->parentFunc;
}
return func;
}

Func *
Func::GetTopFunc()
{
Func * func = this;
while (!func->IsTopFunc())
{
func = func->parentFunc;
}
return func;
}

StackSym *
Func::EnsureLoopParamSym()
{
Expand Down
28 changes: 26 additions & 2 deletions lib/Backend/Func.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,8 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
}
void NumberInstrs();
bool IsTopFunc() const { return this->parentFunc == nullptr; }
Func const * GetTopFunc() const;
Func * GetTopFunc();
Func const * GetTopFunc() const { return this->topFunc; }
Func * GetTopFunc() { return this->topFunc; }

void SetFirstArgOffset(IR::Instr* inlineeStart);

Expand Down Expand Up @@ -732,6 +732,7 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
bool hasTempObjectProducingInstr:1; // At least one instruction which can produce temp object
bool isTJLoopBody : 1;
bool isFlowGraphValid : 1;
bool legalizePostRegAlloc : 1;
#if DBG
bool hasCalledSetDoFastPaths:1;
bool isPostLower:1;
Expand Down Expand Up @@ -836,6 +837,8 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
}
}

bool ShouldLegalizePostRegAlloc() const { return topFunc->legalizePostRegAlloc; }

bool GetApplyTargetInliningRemovedArgumentsAccess() const { return this->applyTargetInliningRemovedArgumentsAccess;}
void SetApplyTargetInliningRemovedArgumentsAccess() { this->applyTargetInliningRemovedArgumentsAccess = true;}

Expand Down Expand Up @@ -1024,6 +1027,7 @@ static const unsigned __int64 c_debugFillPattern8 = 0xcececececececece;
#ifdef PROFILE_EXEC
Js::ScriptContextProfiler *const m_codeGenProfiler;
#endif
Func * const topFunc;
Func * const parentFunc;
StackSym * m_inlineeFrameStartSym;
uint maxInlineeArgOutSize;
Expand Down Expand Up @@ -1106,6 +1110,26 @@ class AutoCodeGenPhase
bool dump;
bool isPhaseComplete;
};

class AutoRestoreLegalize
{
public:
AutoRestoreLegalize(Func * func, bool val) :
m_func(func->GetTopFunc()),
m_originalValue(m_func->legalizePostRegAlloc)
{
m_func->legalizePostRegAlloc = val;
}

~AutoRestoreLegalize()
{
m_func->legalizePostRegAlloc = m_originalValue;
}
private:
Func * m_func;
bool m_originalValue;
};

#define BEGIN_CODEGEN_PHASE(func, phase) { AutoCodeGenPhase __autoCodeGen(func, phase);
#define END_CODEGEN_PHASE(func, phase) __autoCodeGen.EndPhase(func, phase, true, true); }
#define END_CODEGEN_PHASE_NO_DUMP(func, phase) __autoCodeGen.EndPhase(func, phase, false, true); }
Expand Down
3 changes: 2 additions & 1 deletion lib/Backend/LinearScan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4797,7 +4797,8 @@ IR::Instr* LinearScan::InsertLea(IR::RegOpnd *dst, IR::Opnd *src, IR::Instr *con
{
IR::Instr *instrPrev = insertBeforeInstr->m_prev;

IR::Instr *instrRet = Lowerer::InsertLea(dst, src, insertBeforeInstr, true);
AutoRestoreLegalize restore(insertBeforeInstr->m_func, true);
IR::Instr *instrRet = Lowerer::InsertLea(dst, src, insertBeforeInstr);

for (IR::Instr *instr = instrPrev->m_next; instr != insertBeforeInstr; instr = instr->m_next)
{
Expand Down
14 changes: 7 additions & 7 deletions lib/Backend/Lower.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15381,7 +15381,7 @@ IR::Instr *Lowerer::InsertSub(
return instr;
}

IR::Instr *Lowerer::InsertLea(IR::RegOpnd *const dst, IR::Opnd *const src, IR::Instr *const insertBeforeInstr, bool postRegAlloc)
IR::Instr *Lowerer::InsertLea(IR::RegOpnd *const dst, IR::Opnd *const src, IR::Instr *const insertBeforeInstr)
{
Assert(dst);
Assert(src);
Expand All @@ -15393,11 +15393,11 @@ IR::Instr *Lowerer::InsertLea(IR::RegOpnd *const dst, IR::Opnd *const src, IR::I
IR::Instr *const instr = IR::Instr::New(LowererMD::MDLea, dst, src, func);

insertBeforeInstr->InsertBefore(instr);
return ChangeToLea(instr, postRegAlloc);
return ChangeToLea(instr);
}

IR::Instr *
Lowerer::ChangeToLea(IR::Instr * instr, bool postRegAlloc)
Lowerer::ChangeToLea(IR::Instr * instr)
{
Assert(instr);
Assert(instr->GetDst());
Expand All @@ -15407,7 +15407,7 @@ Lowerer::ChangeToLea(IR::Instr * instr, bool postRegAlloc)
Assert(!instr->GetSrc2());

instr->m_opcode = LowererMD::MDLea;
LowererMD::Legalize(instr, postRegAlloc);
LowererMD::Legalize(instr);
return instr;
}

Expand Down Expand Up @@ -27190,7 +27190,7 @@ Lowerer::SetHasBailedOut(IR::Instr * bailoutInstr)
IR::SymOpnd * hasBailedOutOpnd = IR::SymOpnd::New(this->m_func->m_hasBailedOutSym, TyUint32, this->m_func);
IR::Instr * setInstr = IR::Instr::New(LowererMD::GetStoreOp(TyUint32), hasBailedOutOpnd, IR::IntConstOpnd::New(1, TyUint32, this->m_func), this->m_func);
bailoutInstr->InsertBefore(setInstr);
LowererMD::Legalize(setInstr, true);
LowererMD::Legalize(setInstr);
}

IR::Instr*
Expand Down Expand Up @@ -27241,7 +27241,7 @@ Lowerer::EmitSaveEHBailoutReturnValueAndJumpToRetThunk(IR::Instr * insertAfterIn
IR::RegOpnd *eaxOpnd = IR::RegOpnd::New(NULL, LowererMD::GetRegReturn(TyMachReg), TyMachReg, this->m_func);
IR::Instr * movInstr = IR::Instr::New(LowererMD::GetStoreOp(TyVar), bailoutReturnValueSymOpnd, eaxOpnd, this->m_func);
insertAfterInstr->InsertAfter(movInstr);
LowererMD::Legalize(movInstr, true);
LowererMD::Legalize(movInstr);

IR::BranchInstr * jumpInstr = IR::BranchInstr::New(LowererMD::MDUncondBranchOpcode, this->currentRegion->GetBailoutReturnThunkLabel(), this->m_func);
movInstr->InsertAfter(jumpInstr);
Expand All @@ -27263,7 +27263,7 @@ Lowerer::EmitRestoreReturnValueFromEHBailout(IR::LabelInstr * restoreLabel, IR::

epilogLabel->InsertBefore(restoreLabel);
epilogLabel->InsertBefore(movInstr);
LowererMD::Legalize(movInstr, true);
LowererMD::Legalize(movInstr);
restoreLabel->InsertBefore(IR::BranchInstr::New(LowererMD::MDUncondBranchOpcode, epilogLabel, this->m_func));
}

Expand Down
4 changes: 2 additions & 2 deletions lib/Backend/Lower.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,8 @@ class Lowerer
static IR::BranchInstr * InsertTestBranch(IR::Opnd *const testSrc1, IR::Opnd *const testSrc2, const Js::OpCode branchOpCode, const bool isUnsigned, IR::LabelInstr *const target, IR::Instr *const insertBeforeInstr);
static IR::Instr * InsertAdd(const bool needFlags, IR::Opnd *const dst, IR::Opnd *src1, IR::Opnd *src2, IR::Instr *const insertBeforeInstr);
static IR::Instr * InsertSub(const bool needFlags, IR::Opnd *const dst, IR::Opnd *src1, IR::Opnd *src2, IR::Instr *const insertBeforeInstr);
static IR::Instr * InsertLea(IR::RegOpnd *const dst, IR::Opnd *const src, IR::Instr *const insertBeforeInstr, bool postRegAlloc = false);
static IR::Instr * ChangeToLea(IR::Instr *const instr, bool postRegAlloc = false);
static IR::Instr * InsertLea(IR::RegOpnd *const dst, IR::Opnd *const src, IR::Instr *const insertBeforeInstr);
static IR::Instr * ChangeToLea(IR::Instr *const instr);
static IR::Instr * InsertXor(IR::Opnd *const dst, IR::Opnd *const src1, IR::Opnd *const src2, IR::Instr *const insertBeforeInstr);
static IR::Instr * InsertAnd(IR::Opnd *const dst, IR::Opnd *const src1, IR::Opnd *const src2, IR::Instr *const insertBeforeInstr);
static IR::Instr * InsertOr(IR::Opnd *const dst, IR::Opnd *const src1, IR::Opnd *const src2, IR::Instr *const insertBeforeInstr);
Expand Down
4 changes: 2 additions & 2 deletions lib/Backend/arm/EncoderMD.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2401,7 +2401,7 @@ bool EncoderMD::TryConstFold(IR::Instr *instr, IR::RegOpnd *regOpnd)
}

instr->ReplaceSrc(regOpnd, regOpnd->m_sym->GetConstOpnd());
LegalizeMD::LegalizeInstr(instr, false);
LegalizeMD::LegalizeInstr(instr);

return true;
}
Expand All @@ -2421,7 +2421,7 @@ bool EncoderMD::TryFold(IR::Instr *instr, IR::RegOpnd *regOpnd)
}
IR::SymOpnd *symOpnd = IR::SymOpnd::New(regOpnd->m_sym, regOpnd->GetType(), instr->m_func);
instr->ReplaceSrc(regOpnd, symOpnd);
LegalizeMD::LegalizeInstr(instr, false);
LegalizeMD::LegalizeInstr(instr);

return true;
}
Expand Down
Loading

0 comments on commit 4964fc4

Please sign in to comment.