Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Handle cases where shift-amount is 0
  • Loading branch information
kunalspathak committed Jun 1, 2021
commit 53c4325c8299b131d364e528c2ec450e59651f8f
4 changes: 3 additions & 1 deletion src/coreclr/jit/clrjit.natvis
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ The .NET Foundation licenses this file to you under the MIT license.

<Type Name="emitter::instrDesc">
<DisplayString Condition="(_idInsFmt == IF_RRD) || (_idInsFmt == IF_RWR) || (_idInsFmt == IF_RRW)">{_idIns,en} {_idReg1,en}</DisplayString>
<DisplayString Condition="(_idInsFmt == IF_RRD_CNS) || (_idInsFmt == IF_RWR_CNS) || (_idInsFmt == IF_RRW_CNS) || (_idInsFmt == IF_RRW_SHF)">{_idIns,en} {_idReg1,en}, {_idLargeCns,d}</DisplayString>
<DisplayString Condition="(_idInsFmt == IF_RRD_CNS) || (_idInsFmt == IF_RWR_CNS) || (_idInsFmt == IF_RRW_CNS)">{_idIns,en} {_idReg1,en}, {_idLargeCns,d}</DisplayString>
<DisplayString Condition="(_idInsFmt == IF_RRW_SHF) &amp;&amp; (_idLargeCns != 0)">{_idIns,en} {_idReg1,en}, {_idLargeCns,d}</DisplayString>
<DisplayString Condition="(_idInsFmt == IF_RRW_SHF) &amp;&amp; (_idLargeCns == 0)">{_idIns,en} {_idReg1,en}, {_idSmallCns,d}</DisplayString>
<DisplayString>{_idIns,en}</DisplayString>
</Type>

Expand Down
66 changes: 64 additions & 2 deletions src/coreclr/jit/emitxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,69 @@ bool emitter::IsWriteZFFlags(instruction ins)
return (CodeGenInterface::instInfo[ins] & INS_FLAGS_WritesZF) != 0;
}


//------------------------------------------------------------------------
// IsFlagsModified: check if the instruction modifies the flags.
//
// Arguments:
// id - instruction to test
//
// Return Value:
// true if instruction modified any flag, false otherwise.
//
bool emitter::IsFlagsModified(instrDesc* id)
{
instruction ins = id->idIns();
insFormat fmt = id->idInsFmt();

if (fmt == IF_RRW_SHF)
{
if (id->idIsLargeCns())
{
return true;
}
else if (id->idSmallCns() == 0)
{
switch (ins)
{
// If shift-amount for below instructions is 0, then flags are unaffected.
case INS_rcl_N:
case INS_rcr_N:
case INS_rol_N:
case INS_ror_N:
case INS_shl_N:
case INS_shr_N:
case INS_sar_N:
return false;
default:
return true;

}
}
}
else if (fmt == IF_RRW)
{
switch (ins)
{
// If shift-amount for below instructions is 0, then flags are unaffected.
// So, to be conservative, do not optimize if the instruction has register
// as the shift-amount operand.
case INS_rcl:
case INS_rcr:
case INS_rol:
case INS_ror:
case INS_shl:
case INS_shr:
case INS_sar:
return false;
Copy link
Member

@tannergooding tannergooding Jun 2, 2021

Choose a reason for hiding this comment

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

I think this might require some careful considerations.

For the purposes of this PR, this is fine and is simply used as a "did the previous instruction set the flag and therefore a test can be skipped" check.

However, for future improvements we may consider more advanced analysis such as:

if (!IsFlagsModified(prevInstruction))
{
   // can depend on prevInstruction - 1 state being valid 
}

Then, this will become problematic. The latter happens in Clang/MSVC in various places and are one of the reasons that mulx, rorx, sarx, shlx, and shrx are exposed on modern hardware.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can return some ternary state here representing "yes", "no", and "it depends"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Today, we only track prevInstruction (emitLastIns). The way I read your comment, we will need to have access to last couple of instructions (if not more) to make sure the state is valid?

Copy link
Member

Choose a reason for hiding this comment

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

Right, it's not something I think we need to be concerned with "today", but rather something I could see being problematic in the future for certain optimizations.

The method implies boolean state but in practice its slightly more nuanced and one of the answers is "maybe", so it may end up confusing callers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method implies boolean state but in practice its slightly more nuanced and one of the answers is "maybe", so it may end up confusing callers.

Exactly - what are callers supposed to do with "maybe". So for now, I will leave it as-is.

Copy link
Member

Choose a reason for hiding this comment

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

I think that is contextual. In the case of this PR, you care "did the last instruction definitely set the flag I care about" and so you would treat maybe as false.

In other cases, optimizations may care "did the last instruction definitely not set the flag I care about" and so they would treat it as true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the method to IsFlagsAlwaysModified.

default:
return true;
}
}

return true;
}

//------------------------------------------------------------------------
// AreUpper32BitsZero: check if some previously emitted
// instruction set the upper 32 bits of reg to zero.
Expand Down Expand Up @@ -276,7 +339,6 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr
case IF_RRD:
case IF_RRW:
break;

default:
return false;
}
Expand All @@ -294,7 +356,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr

if ((treeOps == GT_EQ) || (treeOps == GT_NE))
{
if (IsWriteZFFlags(lastIns))
if (IsWriteZFFlags(lastIns) && IsFlagsModified(id))
{
return id->idOpSize() == opSize;
}
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/emitxarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ void SetContains256bitAVX(bool value)
bool IsDstDstSrcAVXInstruction(instruction ins);
bool IsDstSrcSrcAVXInstruction(instruction ins);
bool IsWriteZFFlags(instruction ins);
bool IsFlagsModified(instrDesc* id);

bool IsThreeOperandAVXInstruction(instruction ins)
{
return (IsDstDstSrcAVXInstruction(ins) || IsDstSrcSrcAVXInstruction(ins));
Expand Down