Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegen.h
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ class CodeGen final : public CodeGenInterface
void genCompareFloat(GenTreeOp* treeNode);
void genCompareInt(GenTreeOp* treeNode);
#ifdef TARGET_XARCH
bool genCanAvoidEmittingCompareAgainstZero(GenTree* tree, var_types opType);
bool genCanAvoidEmittingCompareAgainstZero(GenTree* tree, emitAttr opSize);
GenTree* genTryFindFlagsConsumer(GenTree* flagsProducer, GenCondition** condition);
#endif

Expand Down
69 changes: 15 additions & 54 deletions src/coreclr/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6414,7 +6414,7 @@ void CodeGen::genCompareInt(GenTreeOp* treeNode)
assert(!varTypeIsFloating(op2Type));

instruction ins;
var_types type = TYP_UNKNOWN;
emitAttr size = EA_ATTR(tree->GetCompareSize());

if (tree->OperIs(GT_TEST_EQ, GT_TEST_NE, GT_TEST))
{
Expand All @@ -6433,18 +6433,12 @@ void CodeGen::genCompareInt(GenTreeOp* treeNode)
#endif
(op2->IsCnsIntOrI() && FitsIn<uint8_t>(op2->AsIntCon()->IconValue())))
{
type = TYP_UBYTE;
size = EA_1BYTE;
}
}
else if (tree->OperIs(GT_BITTEST_EQ, GT_BITTEST_NE, GT_BT))
{
ins = INS_bt;

// BT is a bit special in that the index is used modulo 32. We allow
// mixing the types of op1/op2 because of that -- even if the index is
// TYP_INT but the op size is TYP_LONG the instruction itself will
// ignore the upper part of the register anyway.
type = genActualType(op1->TypeGet());
}
else if (op1->isUsedFromReg() && op2->IsIntegralConst(0))
{
Expand Down Expand Up @@ -6480,49 +6474,17 @@ void CodeGen::genCompareInt(GenTreeOp* treeNode)
ins = INS_cmp;
}

if (type == TYP_UNKNOWN)
{
if (op1Type == op2Type)
{
type = op1Type;
}
else if (genTypeSize(op1Type) == genTypeSize(op2Type))
{
// If the types are different but have the same size then we'll use TYP_INT or TYP_LONG.
// This primarily deals with small type mixes (e.g. byte/ubyte) that need to be widened
// and compared as int. We should not get long type mixes here but handle that as well
// just in case.
type = genTypeSize(op1Type) == 8 ? TYP_LONG : TYP_INT;
}
else
{
// In the types are different simply use TYP_INT. This deals with small type/int type
// mixes (e.g. byte/short ubyte/int) that need to be widened and compared as int.
// Lowering is expected to handle any mixes that involve long types (e.g. int/long).
type = TYP_INT;
}

// The common type cannot be smaller than any of the operand types, we're probably mixing int/long
assert(genTypeSize(type) >= max(genTypeSize(op1Type), genTypeSize(op2Type)));
// Small unsigned int types (TYP_BOOL can use anything) should use unsigned comparisons
assert(!(varTypeIsSmall(type) && varTypeIsUnsigned(type)) || tree->IsUnsigned());
// If op1 is smaller then it cannot be in memory, we're probably missing a cast
assert((genTypeSize(op1Type) >= genTypeSize(type)) || !op1->isUsedFromMemory());
// If op2 is smaller then it cannot be in memory, we're probably missing a cast
assert((genTypeSize(op2Type) >= genTypeSize(type)) || !op2->isUsedFromMemory());
// If we ended up with a small type and op2 is a constant then make sure we don't lose constant bits
assert(!op2->IsCnsIntOrI() || !varTypeIsSmall(type) || FitsIn(type, op2->AsIntCon()->IconValue()));
}

// The type cannot be larger than the machine word size
assert(genTypeSize(type) <= genTypeSize(TYP_I_IMPL));
// TYP_UINT and TYP_ULONG should not appear here, only small types can be unsigned
assert(!varTypeIsUnsigned(type) || varTypeIsSmall(type));
assert((genTypeSize(op1Type) >= EA_SIZE_IN_BYTES(size)) || !op1->isUsedFromMemory());
assert((genTypeSize(op2Type) >= EA_SIZE_IN_BYTES(size)) || !op2->isUsedFromMemory());
assert(!op2->IsCnsIntOrI() || (size != EA_1BYTE) || FitsIn<int8_t>(op2->AsIntCon()->IconValue()) ||
FitsIn<uint8_t>(op2->AsIntCon()->IconValue()));
assert(!op2->IsCnsIntOrI() || (size != EA_2BYTE) || FitsIn<int16_t>(op2->AsIntCon()->IconValue()) ||
FitsIn<uint16_t>(op2->AsIntCon()->IconValue()));
assert(size <= EA_PTRSIZE);

if (!canReuseFlags || !genCanAvoidEmittingCompareAgainstZero(tree, type))
if (!canReuseFlags || !genCanAvoidEmittingCompareAgainstZero(tree, size))
{
emitAttr size = emitTypeSize(type);
bool canSkip = m_compiler->opts.OptimizationEnabled() && (ins == INS_cmp) && !op1->isUsedFromMemory() &&
bool canSkip = m_compiler->opts.OptimizationEnabled() && (ins == INS_cmp) && !op1->isUsedFromMemory() &&
!op2->isUsedFromMemory() && emit->IsRedundantCmp(size, op1->GetRegNum(), op2->GetRegNum());

if (!canSkip)
Expand All @@ -6546,12 +6508,12 @@ void CodeGen::genCompareInt(GenTreeOp* treeNode)
//
// Parameters:
// tree - the compare node
// opType - type of the compare
// opSize - operand size of the compare
//
// Returns:
// True if the compare can be omitted.
//
bool CodeGen::genCanAvoidEmittingCompareAgainstZero(GenTree* tree, var_types opType)
bool CodeGen::genCanAvoidEmittingCompareAgainstZero(GenTree* tree, emitAttr opSize)
{
GenTree* op1 = tree->gtGetOp1();
assert(tree->gtGetOp2()->IsIntegralConst(0));
Expand Down Expand Up @@ -6580,14 +6542,13 @@ bool CodeGen::genCanAvoidEmittingCompareAgainstZero(GenTree* tree, var_types opT
cond = *mutableCond;
}

if (GetEmitter()->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(opType), cond))
if (GetEmitter()->AreFlagsSetToZeroCmp(op1->GetRegNum(), opSize, cond))
{
JITDUMP("Not emitting compare due to flags being already set\n");
return true;
}

if ((mutableCond != nullptr) &&
GetEmitter()->AreFlagsSetForSignJumpOpt(op1->GetRegNum(), emitTypeSize(opType), cond))
if ((mutableCond != nullptr) && GetEmitter()->AreFlagsSetForSignJumpOpt(op1->GetRegNum(), opSize, cond))
{
JITDUMP("Not emitting compare due to sign being already set; modifying [%06u] to check sign flag\n",
Compiler::dspTreeID(consumer));
Expand Down
55 changes: 55 additions & 0 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3919,6 +3919,61 @@ GenTree* Compiler::gtReverseCond(GenTree* tree)
return tree;
}

#ifdef TARGET_XARCH
//------------------------------------------------------------------------
// GetCompareSize: Get the operand size of a lowered xarch comparison.
//
// Return Value:
// The comparison width in bytes.
//
// Remarks:
// Requires completed containment checking. Mixed signed/unsigned small types
// use 4 bytes unless containment has proved that both values fit the memory
// operand's range, in which case its size is used (e.g. 2 bytes for a contained
// short compared with a byte). Without that proof, TYP_BYTE versus TYP_UBYTE
// uses 4 bytes, preserving sign/zero extension so -1 and 255 remain distinct.
// GTF_UNSIGNED, not this size, determines the comparison's signedness.
//
// Codegen may further narrow TEST immediates when the allocated registers
// support a byte operation.
//
unsigned GenTreeOp::GetCompareSize() const
{
assert(OperIsCompare() || OperIs(GT_CMP, GT_TEST, GT_BT));

var_types op1Type = gtOp1->TypeGet();
var_types op2Type = gtOp2->TypeGet();

if (OperIs(GT_BITTEST_EQ, GT_BITTEST_NE, GT_BT))
{
// BT uses the index modulo the operand width, so its type is independent
// of the index's type.
return genTypeSize(genActualType(op1Type));
}

if (gtOp1->isContained() && !gtOp1->IsCnsIntOrI())
{
return genTypeSize(op1Type);
}

if (gtOp2->isContained() && !gtOp2->IsCnsIntOrI())
{
return genTypeSize(op2Type);
}

if (op1Type == op2Type)
{
return genTypeSize(op1Type);
}

// Different small types must be extended before comparison. Mixes involving
// long operands must have been handled by lowering.
unsigned size = ((genTypeSize(op1Type) == 8) && (genTypeSize(op2Type) == 8)) ? 8 : 4;
assert(size >= max(genTypeSize(op1Type), genTypeSize(op2Type)));
return size;
}
#endif // TARGET_XARCH

#if !defined(TARGET_64BIT) || defined(TARGET_ARM64)
//------------------------------------------------------------------------------
// IsValidLongMul : Check for long multiplication with 32 bit operands.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -3251,6 +3251,10 @@ struct GenTreeOp : public GenTreeUnOp
// then sets the flag GTF_DIV_BY_CNS_OPT and GTF_DONT_CSE on the constant
void CheckDivideByConstOptimized(Compiler* comp);

#ifdef TARGET_XARCH
unsigned GetCompareSize() const;
#endif // TARGET_XARCH

GenTree*& ReturnValueRef()
{
assert(OperIs(GT_RETURN, GT_RETFILT, GT_SWIFT_ERROR_RET));
Expand Down
36 changes: 2 additions & 34 deletions src/coreclr/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4198,7 +4198,6 @@ GenTree* Lowering::DecomposeLongCompare(GenTree* cmp)
// longer needed.
//
// Notes:
// - Narrow operands to enable memory operand containment (XARCH specific).
// - Transform cmp(and(x, y), 0) into test(x, y) (XARCH/Arm64 specific but could
// be used for ARM as well if support for GT_TEST_EQ/GT_TEST_NE is added).
// - Transform TEST(x, LSH(1, y)) into BT(x, y) (XARCH specific)
Expand Down Expand Up @@ -4274,22 +4273,7 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp)

INT64 op2Value = op2->IntegralValue();

#ifdef TARGET_XARCH
var_types op1Type = op1->TypeGet();
if (IsContainableMemoryOp(op1) && varTypeIsSmall(op1Type) && FitsIn(op1Type, op2Value))
{
//
// If op1's type is small then try to narrow op2 so it has the same type as op1.
// Small types are usually used by memory loads and if both compare operands have
// the same type then the memory load can be contained. In certain situations
// (e.g "cmp ubyte, 200") we also get a smaller instruction encoding.
//

op2->gtType = op1Type;
}
else
#endif
if (op1->OperIs(GT_CAST) && !op1->gtOverflow())
if (op1->OperIs(GT_CAST) && !op1->gtOverflow())
{
GenTreeCast* cast = op1->AsCast();
var_types castToType = cast->CastToType();
Expand Down Expand Up @@ -4687,23 +4671,7 @@ GenTree* Lowering::LowerCompare(GenTree* cmp)
}
}

#ifdef TARGET_XARCH
if (cmp->gtGetOp1()->TypeGet() == cmp->gtGetOp2()->TypeGet())
{
if (varTypeIsSmall(cmp->gtGetOp1()->TypeGet()) && varTypeIsUnsigned(cmp->gtGetOp1()->TypeGet()))
{
//
// If both operands have the same type then codegen will use the common operand type to
// determine the instruction type. For small types this would result in performing a
// signed comparison of two small unsigned values without zero extending them to TYP_INT
// which is incorrect. Note that making the comparison unsigned doesn't imply that codegen
// has to generate a small comparison, it can still correctly generate a TYP_INT comparison.
//

cmp->SetUnsigned();
}
}
#elif defined(TARGET_RISCV64)
#ifdef TARGET_RISCV64
if (varTypeUsesIntReg(cmp->gtGetOp1()))
{
if (GenTree* next = LowerSavedIntegerCompare(cmp); next != cmp)
Expand Down
44 changes: 40 additions & 4 deletions src/coreclr/jit/lowerxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8317,6 +8317,27 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
// TODO-XArch-CQ: factor out cmp optimization in 'genCondSetFlags' to be used here
// or in other backend.

auto canCompareAtMemoryWidth = [this, cmp](GenTree* memoryOp, GenTree* otherOp) {
if (memoryOp->TypeGet() == otherOp->TypeGet())
{
return true;
}

if (!cmp->OperIsCmpCompare() || !m_compiler->opts.OptimizationEnabled() || !varTypeIsSmall(memoryOp) ||
!varTypeIsIntegral(otherOp))
{
return false;
}

IntegralRange range = IntegralRange::ForType(memoryOp->TypeGet());
if (otherOp->IsIntegralConst())
{
return range.Contains(otherOp->AsIntConCommon()->IntegralValue());
}

return range.Contains(IntegralRange::ForNode(otherOp, m_compiler));
Comment thread
jakobbotsch marked this conversation as resolved.
Comment thread
jakobbotsch marked this conversation as resolved.
};

if (CheckImmedAndMakeContained(cmp, op2))
{
// If the types are the same, or if the constant is of the correct size,
Expand All @@ -8325,8 +8346,12 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
{
TryMakeSrcContainedOrRegOptional(cmp, op1);
}
else if (IsContainableMemoryOp(op1) && canCompareAtMemoryWidth(op1, op2) && IsSafeToContainMem(cmp, op1))
{
MakeSrcContained(cmp, op1);
}
}
else if (op1Type == op2Type)
else
{
// Note that TEST does not have a r,rm encoding like CMP has but we can still
// contain the second operand because the emitter maps both r,rm and rm,r to
Expand All @@ -8335,7 +8360,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
bool isSafeToContainOp1 = true;
bool isSafeToContainOp2 = true;

if (IsContainableMemoryOp(op2))
if (IsContainableMemoryOp(op2) && canCompareAtMemoryWidth(op2, op1))
{
isSafeToContainOp2 = IsSafeToContainMem(cmp, op2);
if (isSafeToContainOp2)
Expand All @@ -8344,7 +8369,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
}
}

if (!op2->isContained() && IsContainableMemoryOp(op1))
if (!op2->isContained() && IsContainableMemoryOp(op1) && canCompareAtMemoryWidth(op1, op2))
{
isSafeToContainOp1 = IsSafeToContainMem(cmp, op1);
if (isSafeToContainOp1)
Expand All @@ -8353,7 +8378,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
}
}

if (!op1->isContained() && !op2->isContained())
if ((op1Type == op2Type) && !op1->isContained() && !op2->isContained())
{
// One of op1 or op2 could be marked as reg optional
// to indicate that codegen can still generate code
Expand All @@ -8368,6 +8393,17 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
}
}
}

// A contained memory operand bounds both values; otherwise small compares
// have matching operand types.
GenTree* rangeSource = (op2->isContained() && !op2->IsCnsIntOrI()) ? op2 : op1;
if (cmp->OperIsCompare() && (cmp->GetCompareSize() < genTypeSize(TYP_INT)) && varTypeIsUnsigned(rangeSource))
{
// A small compare uses a lower-numbered sign bit. Use unsigned conditions
// when zero extension would have made the wider sign bit zero.
// For example, byte 200 < 100 is false, but signed byte -56 < 100 is true.
cmp->SetUnsigned();
}
Comment thread
jakobbotsch marked this conversation as resolved.
}

//------------------------------------------------------------------------
Expand Down
32 changes: 1 addition & 31 deletions src/coreclr/jit/lsrabuild.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4608,37 +4608,7 @@ int LinearScan::BuildCmpOperands(GenTree* tree)
GenTree* op2 = tree->gtGetOp2();

#ifdef TARGET_X86
bool needByteRegs = false;
if (varTypeIsByte(tree))
{
if (varTypeUsesIntReg(op1))
{
needByteRegs = true;
}
}
// Example1: GT_EQ(int, op1 of type ubyte, op2 of type ubyte) - in this case codegen uses
// ubyte as the result of comparison and if the result needs to be materialized into a reg
// simply zero extend it to TYP_INT size. Here is an example of generated code:
// cmp dl, byte ptr[addr mode]
// movzx edx, dl
else if (varTypeIsByte(op1) && varTypeIsByte(op2))
{
needByteRegs = true;
}
// Example2: GT_EQ(int, op1 of type ubyte, op2 is GT_CNS_INT) - in this case codegen uses
// ubyte as the result of the comparison and if the result needs to be materialized into a reg
// simply zero extend it to TYP_INT size.
else if (varTypeIsByte(op1) && op2->IsCnsIntOrI())
{
needByteRegs = true;
}
// Example3: GT_EQ(int, op1 is GT_CNS_INT, op2 of type ubyte) - in this case codegen uses
// ubyte as the result of the comparison and if the result needs to be materialized into a reg
// simply zero extend it to TYP_INT size.
else if (op1->IsCnsIntOrI() && varTypeIsByte(op2))
{
needByteRegs = true;
}
bool needByteRegs = (varTypeIsByte(tree) && varTypeUsesIntReg(op1)) || (tree->AsOp()->GetCompareSize() == 1);
if (needByteRegs)
{
if (!op1->isContained())
Expand Down
Loading