Skip to content

[draft] Use csel for conditional moves #67286

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

Closed
a74nh wants to merge 1 commit into from
Closed

[draft] Use csel for conditional moves #67286

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
1 change: 1 addition & 0 deletions src/coreclr/jit/codegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
void genCodeForLclFld(GenTreeLclFld* tree);
void genCodeForStoreLclFld(GenTreeLclFld* tree);
void genCodeForStoreLclVar(GenTreeLclVar* tree);
void genCodeForCStoreLclVar(GenTreeLclVar* tree);
void genCodeForReturnTrap(GenTreeOp* tree);
void genCodeForJcc(GenTreeCC* tree);
void genCodeForSetcc(GenTreeCC* setcc);
Expand Down
33 changes: 33 additions & 0 deletions src/coreclr/jit/codegenarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2647,6 +2647,39 @@ void CodeGen::genCodeForStoreLclVar(GenTreeLclVar* lclNode)
}
}

//------------------------------------------------------------------------
// genCodeForStoreLclVar: Produce code for a GT_CSTORE_LCL_VAR node.
//
// Arguments:
// lclNode - the GT_CSTORE_LCL_VAR node
//
void CodeGen::genCodeForCStoreLclVar(GenTreeLclVar* lclNode)
{
emitter* emit = GetEmitter();

insCond cond = ((lclNode->gtFlags & GTF_CSEL_T) != 0) ? INS_COND_EQ : INS_COND_NE;

emitAttr attr = emitActualTypeSize(lclNode->TypeGet());

// AHTODO: do we need this?
// // TODO-CQ: Currently we cannot do this for all handles because of
// // https://github.com/dotnet/runtime/issues/60712
// if (con->ImmedValNeedsReloc(compiler))
// {
// attr = EA_SET_FLG(attr, EA_CNS_RELOC_FLG);
// }
// if (targetType == TYP_BYREF)
// {
// attr = EA_SET_FLG(attr, EA_BYREF_FLG);
// }

regNumber targetReg = lclNode->GetRegNum();
regNumber srcReg = genConsumeReg(lclNode->gtGetOp1());

emit->emitIns_R_R_R_COND(INS_csel, attr, targetReg, srcReg, targetReg, cond);
regSet.verifyRegUsed(targetReg);
}

//------------------------------------------------------------------------
// genSimpleReturn: Generates code for simple return statement for arm64.
//
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
genCodeForStoreLclVar(treeNode->AsLclVar());
break;

case GT_CSTORE_LCL_VAR:
genCodeForCStoreLclVar(treeNode->AsLclVar());
break;

case GT_RETFILT:
case GT_RETURN:
genReturn(treeNode);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9134,6 +9134,7 @@ void cTreeFlags(Compiler* comp, GenTree* tree)
case GT_LCL_FLD_ADDR:
case GT_STORE_LCL_FLD:
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
if (tree->gtFlags & GTF_VAR_DEF)
{
chars += printf("[VAR_DEF]");
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11506,6 +11506,7 @@ class GenTreeVisitor

// Lclvar unary operators
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
case GT_STORE_LCL_FLD:
if (TVisitor::DoLclVarsOnly)
{
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/decomposelongs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ GenTree* DecomposeLongs::DecomposeNode(GenTree* tree)
case GT_STORE_LCL_VAR:
nextNode = DecomposeStoreLclVar(use);
break;
// case GT_CSTORE_LCL_VAR: Do nothing?

case GT_CAST:
nextNode = DecomposeCast(use);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gcinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ GCInfo::WriteBarrierForm GCInfo::gcIsWriteBarrierCandidate(GenTree* tgt, GenTree
case GT_LCL_VAR: /* Definitely not in the managed heap */
case GT_LCL_FLD:
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
case GT_STORE_LCL_FLD:
return WBF_NoBarrier;

Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9600,6 +9600,7 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_
case GT_LCL_FLD_ADDR:
case GT_STORE_LCL_FLD:
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
if (tree->gtFlags & GTF_VAR_USEASG)
{
printf("U");
Expand Down Expand Up @@ -9799,7 +9800,7 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_
{
layout = tree->AsBlk()->GetLayout();
}
else if (tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR))
else if (tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR))
{
LclVarDsc* varDsc = lvaGetDesc(tree->AsLclVar());

Expand Down Expand Up @@ -9844,7 +9845,7 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_
}
}

if (tree->gtOper == GT_LCL_VAR || tree->gtOper == GT_STORE_LCL_VAR)
if (tree->gtOper == GT_LCL_VAR || tree->gtOper == GT_STORE_LCL_VAR || tree->gtOper == GT_CSTORE_LCL_VAR)
{
LclVarDsc* varDsc = lvaGetDesc(tree->AsLclVarCommon());
if (varDsc->IsAddressExposed())
Expand Down Expand Up @@ -10539,6 +10540,7 @@ void Compiler::gtDispLeaf(GenTree* tree, IndentStack* indentStack)
case GT_LCL_VAR:
case GT_LCL_VAR_ADDR:
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
{
printf(" ");
const unsigned varNum = tree->AsLclVarCommon()->GetLclNum();
Expand Down
25 changes: 14 additions & 11 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,9 @@ enum GenTreeFlags : unsigned int
GTF_LATE_ARG = 0x00010000, // The specified node is evaluated to a temp in the arg list, and this temp is added to gtCallLateArgs.
GTF_SPILL = 0x00020000, // Needs to be spilled here

GTF_CSEL_T = 0x00040000, // Conditional op should check for True flags
GTF_CSEL_F = 0x00080000, // Conditional op should check for False flags

// The extra flag GTF_IS_IN_CSE is used to tell the consumer of the side effect flags
// that we are calling in the context of performing a CSE, thus we
// should allow the run-once side effects of running a class constructor.
Expand Down Expand Up @@ -1165,7 +1168,7 @@ struct GenTree
static bool OperIsLocal(genTreeOps gtOper)
{
static_assert_no_msg(
OpersAreContiguous(GT_PHI_ARG, GT_LCL_VAR, GT_LCL_FLD, GT_STORE_LCL_VAR, GT_STORE_LCL_FLD));
OpersAreContiguous(GT_PHI_ARG, GT_LCL_VAR, GT_LCL_FLD, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR, GT_STORE_LCL_FLD));
return (GT_PHI_ARG <= gtOper) && (gtOper <= GT_STORE_LCL_FLD);
}

Expand All @@ -1186,7 +1189,7 @@ struct GenTree

static bool OperIsScalarLocal(genTreeOps gtOper)
{
return (gtOper == GT_LCL_VAR || gtOper == GT_STORE_LCL_VAR);
return (gtOper == GT_LCL_VAR || gtOper == GT_STORE_LCL_VAR || gtOper == GT_CSTORE_LCL_VAR);
}

static bool OperIsNonPhiLocal(genTreeOps gtOper)
Expand All @@ -1201,7 +1204,7 @@ struct GenTree

static bool OperIsLocalStore(genTreeOps gtOper)
{
return (gtOper == GT_STORE_LCL_VAR || gtOper == GT_STORE_LCL_FLD);
return (gtOper == GT_STORE_LCL_VAR || gtOper == GT_STORE_LCL_FLD || gtOper == GT_CSTORE_LCL_VAR);
}

static bool OperIsAddrMode(genTreeOps gtOper)
Expand Down Expand Up @@ -1573,7 +1576,7 @@ struct GenTree

static bool OperIsStore(genTreeOps gtOper)
{
return (gtOper == GT_STOREIND || gtOper == GT_STORE_LCL_VAR || gtOper == GT_STORE_LCL_FLD ||
return (gtOper == GT_STOREIND || gtOper == GT_STORE_LCL_VAR || gtOper == GT_CSTORE_LCL_VAR || gtOper == GT_STORE_LCL_FLD ||
OperIsStoreBlk(gtOper) || OperIsAtomicOp(gtOper));
}

Expand Down Expand Up @@ -2934,7 +2937,7 @@ struct GenTreeOp : public GenTreeUnOp
: GenTreeUnOp(oper, type DEBUGARG(largeNode)), gtOp2(nullptr)
{
// Unary operators with optional arguments:
assert(oper == GT_NOP || oper == GT_RETURN || oper == GT_RETFILT || OperIsBlk(oper));
assert(oper == GT_NOP || oper == GT_RETURN || oper == GT_RETFILT || OperIsBlk(oper) || OperIsLocal(oper) || OperIsLocalAddr(oper));
}

// returns true if we will use the division by constant optimization for this node.
Expand Down Expand Up @@ -3298,16 +3301,16 @@ struct GenTreeStrCon : public GenTree
};

// Common supertype of LCL_VAR, LCL_FLD, REG_VAR, PHI_ARG
// This inherits from UnOp because lclvar stores are Unops
struct GenTreeLclVarCommon : public GenTreeUnOp
// This inherits from GenTreeOp because CSTORE_LCL_VAR has 2 ops
struct GenTreeLclVarCommon : public GenTreeOp
Comment on lines +3304 to +3305
Copy link
Contributor

@SingleAccretion SingleAccretion Mar 29, 2022

Choose a reason for hiding this comment

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

This is not a viable approach. It increases the size of small nodes by one pointer (80 -> 88 bytes on 64 bit hosts).

Copy link
Contributor Author

@a74nh a74nh Mar 29, 2022

Choose a reason for hiding this comment

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

This is not a viable approach. It increases the size of small nodes by one pointer (80 -> 88 bytes on 64 bit hosts).

If that's the case then that also gets rid of my idea of just using STORE_LCL_VAR nodes for conditional stores (STORE_LCL_VAR would then just check in the emit if a conditional flag is set).

Ok then, I think I can just make a new class:
struct GenTreeCondLclVar : public GenTreeLclVarCommon

Copy link
Contributor

@SingleAccretion SingleAccretion Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What is the reason for attaching this to local stores at all?

I would expect you can just define a new GT_SELECT-like oper, that uses (implicitly, like GT_JCC/GT_SETCC) flags to produce a value.

Copy link
Contributor Author

@a74nh a74nh Mar 29, 2022

Choose a reason for hiding this comment

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

My original instinct was to avoid adding new nodes into the graph, as I was assuming that would result in additional instructions being generated (a csel for the new node, then a mov for the store). But, I'm now realising the register allocator would allocate correctly and the lcl var store would end up as no instruction.

Also, I think my code isn't going to work in cases where the STORE_LCL_VAR needed to store to memory (which probably explains why the c# libraries fail to build)

Copy link
Contributor

@SingleAccretion SingleAccretion Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But, I'm now realising the register allocator would allocate correctly and the lcl var store would end up as no instruction.

Right. "Combining" stores to locals with instructions, if taken to its logical conclusion, would mean adding local stores for all opers, STORE_LCL_VAR_[ADD|SUB|MUL...], so that's not something we should do.

(Well -- as a little history detour -- we used to have legacy codegen that did that, but it was mainly motivated by the RMW semantics of most x86 instructions)

{
private:
unsigned _gtLclNum; // The local number. An index into the Compiler::lvaTable array.
unsigned _gtSsaNum; // The SSA number.

public:
GenTreeLclVarCommon(genTreeOps oper, var_types type, unsigned lclNum DEBUGARG(bool largeNode = false))
: GenTreeUnOp(oper, type DEBUGARG(largeNode))
: GenTreeOp(oper, type DEBUGARG(largeNode))
{
SetLclNum(lclNum);
}
Expand Down Expand Up @@ -3341,7 +3344,7 @@ struct GenTreeLclVarCommon : public GenTreeUnOp
}

#if DEBUGGABLE_GENTREE
GenTreeLclVarCommon() : GenTreeUnOp()
GenTreeLclVarCommon() : GenTreeOp()
{
}
#endif
Expand Down Expand Up @@ -8298,7 +8301,7 @@ inline GenTreeFlags GenTree::GetRegSpillFlagByIdx(int regIndex) const
inline GenTreeFlags GenTree::GetLastUseBit(int regIndex) const
{
assert(regIndex < 4);
assert(OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_COPY, GT_RELOAD));
assert(OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR, GT_COPY, GT_RELOAD));
static_assert_no_msg((1 << MULTIREG_LAST_USE_SHIFT) == GTF_VAR_MULTIREG_DEATH0);
return (GenTreeFlags)(1 << (MULTIREG_LAST_USE_SHIFT + regIndex));
}
Expand All @@ -8317,7 +8320,7 @@ inline GenTreeFlags GenTree::GetLastUseBit(int regIndex) const
//
inline bool GenTree::IsLastUse(int regIndex) const
{
assert(OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_COPY, GT_RELOAD));
assert(OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR, GT_COPY, GT_RELOAD));
return (gtFlags & GetLastUseBit(regIndex)) != 0;
}

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gtlist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ GTNODE(PHI_ARG , GenTreePhiArg ,0,GTK_LEAF) // phi(phi
GTNODE(LCL_VAR , GenTreeLclVar ,0,GTK_LEAF) // local variable
GTNODE(LCL_FLD , GenTreeLclFld ,0,GTK_LEAF) // field in a non-primitive variable
GTNODE(STORE_LCL_VAR , GenTreeLclVar ,0,GTK_UNOP|GTK_NOVALUE) // store to local variable
GTNODE(CSTORE_LCL_VAR , GenTreeLclVar ,0,GTK_BINOP|GTK_NOVALUE) // conditional store to local variable
GTNODE(STORE_LCL_FLD , GenTreeLclFld ,0,GTK_UNOP|GTK_NOVALUE) // store to a part of the variable
GTNODE(LCL_VAR_ADDR , GenTreeLclVar ,0,GTK_LEAF) // address of local variable
GTNODE(LCL_FLD_ADDR , GenTreeLclFld ,0,GTK_LEAF) // address of field in a non-primitive variable
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/gtstructs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ GTSTRUCT_1(IntCon , GT_CNS_INT)
GTSTRUCT_1(LngCon , GT_CNS_LNG)
GTSTRUCT_1(DblCon , GT_CNS_DBL)
GTSTRUCT_1(StrCon , GT_CNS_STR)
GTSTRUCT_N(LclVarCommon, GT_LCL_VAR, GT_LCL_FLD, GT_PHI_ARG, GT_STORE_LCL_VAR, GT_STORE_LCL_FLD, GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR)
GTSTRUCT_3(LclVar , GT_LCL_VAR, GT_LCL_VAR_ADDR, GT_STORE_LCL_VAR)
GTSTRUCT_N(LclVarCommon, GT_LCL_VAR, GT_LCL_FLD, GT_PHI_ARG, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR, GT_STORE_LCL_FLD, GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR)
GTSTRUCT_4(LclVar , GT_LCL_VAR, GT_LCL_VAR_ADDR, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR)
GTSTRUCT_3(LclFld , GT_LCL_FLD, GT_STORE_LCL_FLD, GT_LCL_FLD_ADDR)
GTSTRUCT_1(Cast , GT_CAST)
GTSTRUCT_1(Box , GT_BOX)
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lclvars.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4537,6 +4537,7 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers)
case GT_LCL_VAR_ADDR:
case GT_LCL_FLD_ADDR:
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
case GT_STORE_LCL_FLD:
{
LclVarDsc* varDsc = lvaGetDesc(node->AsLclVarCommon());
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/liveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void Compiler::fgMarkUseDef(GenTreeLclVarCommon* tree)
{
// If this is an enregisterable variable that is not marked doNotEnregister,
// we should only see direct references (not ADDRs).
assert(varDsc->lvDoNotEnregister || tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR));
assert(varDsc->lvDoNotEnregister || tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_CSTORE_LCL_VAR));
}

if (isUse && !VarSetOps::IsMember(this, fgCurDefSet, varDsc->lvVarIndex))
Expand Down Expand Up @@ -220,6 +220,7 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree)
case GT_LCL_VAR_ADDR:
case GT_LCL_FLD_ADDR:
case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
case GT_STORE_LCL_FLD:
fgMarkUseDef(tree->AsLclVarCommon());
break;
Expand Down Expand Up @@ -2013,6 +2014,7 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
break;

case GT_STORE_LCL_VAR:
case GT_CSTORE_LCL_VAR:
case GT_STORE_LCL_FLD:
{
GenTreeLclVarCommon* const lclVarNode = node->AsLclVarCommon();
Expand Down
Loading