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.

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

Arm64/Sve: Remove unwanted insScalableOpts and instructions #103620

Merged
merged 10 commits into from
Jun 18, 2024
483 changes: 208 additions & 275 deletions src/coreclr/jit/codegenarm64test.cpp

Large diffs are not rendered by default.

77 changes: 14 additions & 63 deletions src/coreclr/jit/emitarm64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4252,9 +4252,15 @@ void emitter::emitIns_Mov(
{
if (isPredicateRegister(dstReg) && isPredicateRegister(srcReg))
{
assert(insOptsNone(opt));
if (insOptsNone(opt))
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is odd. This is because for predicates we always use INS_OPTS_SCALABLE_B for a mov, so the passed in opt value doesn't matter ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, so basically, this code operates just on INS_OPTS_SCALABLE_B (and we added single test for that). For other places, if we have to pass INS_OPTS_SCALABLE_B if register types are predicate feels like an overhead, so instead this code expects it to be NONE, in which case it will set the options to INS_OPTS_SCALABLE_B.

Copy link
Member

Choose a reason for hiding this comment

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

Would something like this work?

if (!insOptsNone(opt))
{
    assert(opt == INS_OPTS_SCALABLE_B);
}

opt = INS_OPTS_SCALABLE_B;

I think that shape makes it clearer the if-statement is debug-only code, and ensures opt is always INS_OPTS_SCALABLE_B even if we don't have asserts on to catch improper handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

assert(opt == INS_OPTS_SCALABLE_B || insOptsNone(opt));
opt = INS_OPTS_SCALABLE_B;

?

{
opt = INS_OPTS_SCALABLE_B;
}
else
{
assert(opt == INS_OPTS_SCALABLE_B);
}

opt = INS_OPTS_SCALABLE_B;
attr = EA_SCALABLE;

if (IsRedundantMov(ins, size, dstReg, srcReg, canSkip))
Expand Down Expand Up @@ -7882,42 +7888,14 @@ void emitter::emitIns_R_S(instruction ins, emitAttr attr, regNumber reg1, int va

case INS_sve_ldr:
{
assert(isVectorRegister(reg1));
isSimple = false;
size = EA_SCALABLE;
attr = size;
fmt = IF_SVE_IE_2A;

// TODO-SVE: Don't assume 128bit vectors
scale = NaturalScale_helper(EA_16BYTE);
ssize_t mask = (1 << scale) - 1; // the mask of low bits that must be zero to encode the immediate

if (((imm & mask) == 0) && (isValidSimm<9>(imm >> scale)))
{
imm >>= scale; // The immediate is scaled by the size of the ld/st
}
else
{
useRegForImm = true;
regNumber rsvdReg = codeGen->rsGetRsvdReg();
codeGen->instGen_Set_Reg_To_Imm(EA_PTRSIZE, rsvdReg, imm);
}
}
break;

// TODO-SVE: Fold into INS_sve_ldr once REG_V0 and REG_P0 are distinct
case INS_sve_ldr_mask:
{
assert(isPredicateRegister(reg1));
isSimple = false;
size = EA_SCALABLE;
attr = size;
fmt = IF_SVE_ID_2A;
ins = INS_sve_ldr;
fmt = isVectorRegister(reg1) ? IF_SVE_IE_2A : IF_SVE_ID_2A;

// TODO-SVE: Don't assume 128bit vectors
// Predicate size is vector length / 8
scale = NaturalScale_helper(EA_2BYTE);
scale = NaturalScale_helper(isVectorRegister(reg1) ? EA_16BYTE : EA_2BYTE);
ssize_t mask = (1 << scale) - 1; // the mask of low bits that must be zero to encode the immediate

if (((imm & mask) == 0) && (isValidSimm<9>(imm >> scale)))
Expand All @@ -7930,8 +7908,8 @@ void emitter::emitIns_R_S(instruction ins, emitAttr attr, regNumber reg1, int va
regNumber rsvdReg = codeGen->rsGetRsvdReg();
codeGen->instGen_Set_Reg_To_Imm(EA_PTRSIZE, rsvdReg, imm);
}
break;
}
break;

default:
NYI("emitIns_R_S"); // FP locals?
Expand Down Expand Up @@ -8161,42 +8139,14 @@ void emitter::emitIns_S_R(instruction ins, emitAttr attr, regNumber reg1, int va

case INS_sve_str:
{
assert(isVectorRegister(reg1));
isSimple = false;
size = EA_SCALABLE;
attr = size;
fmt = IF_SVE_JH_2A;

// TODO-SVE: Don't assume 128bit vectors
scale = NaturalScale_helper(EA_16BYTE);
ssize_t mask = (1 << scale) - 1; // the mask of low bits that must be zero to encode the immediate

if (((imm & mask) == 0) && (isValidSimm<9>(imm >> scale)))
{
imm >>= scale; // The immediate is scaled by the size of the ld/st
}
else
{
useRegForImm = true;
regNumber rsvdReg = codeGen->rsGetRsvdReg();
codeGen->instGen_Set_Reg_To_Imm(EA_PTRSIZE, rsvdReg, imm);
}
}
break;

// TODO-SVE: Fold into INS_sve_str once REG_V0 and REG_P0 are distinct
case INS_sve_str_mask:
{
assert(isPredicateRegister(reg1));
isSimple = false;
size = EA_SCALABLE;
attr = size;
fmt = IF_SVE_JG_2A;
ins = INS_sve_str;
fmt = isVectorRegister(reg1) ? IF_SVE_JH_2A : IF_SVE_JG_2A;

// TODO-SVE: Don't assume 128bit vectors
// Predicate size is vector length / 8
scale = NaturalScale_helper(EA_2BYTE);
scale = NaturalScale_helper(isVectorRegister(reg1) ? EA_16BYTE : EA_2BYTE);
ssize_t mask = (1 << scale) - 1; // the mask of low bits that must be zero to encode the immediate

if (((imm & mask) == 0) && (isValidSimm<9>(imm >> scale)))
Expand All @@ -8209,6 +8159,7 @@ void emitter::emitIns_S_R(instruction ins, emitAttr attr, regNumber reg1, int va
regNumber rsvdReg = codeGen->rsGetRsvdReg();
codeGen->instGen_Set_Reg_To_Imm(EA_PTRSIZE, rsvdReg, imm);
}
break;
}
break;

Expand Down
Loading
Loading