Skip to content

Commit 14b5442

Browse files
alexmalyshevmeta-codesync[bot]
alexmalyshev
authored and
meta-codesync[bot]
committed
Collapse MovSX into Sext and MovZX into Zext in LIR
Summary: `Sext`/`Zext` and `MovSX`/`MovZX` were two spellings of one operation. They agree on every opcode property, `writesFlags`, `isEssential`, `inputMustBeRegister`, `outputMustBeRegister` and `operandSizeType`, and both already appear before and after register allocation, the LIR generator emits `Sext`/`Zext`, while boxing, the compare widening in postgen and the divide rewrite emit `MovSX`/`MovZX`. The only real difference was that `rewriteBitExtensionInstrs` renamed the first pair into the second on its way to codegen. Keep `Sext`/`Zext` and drop `MovSX`/`MovZX`. Codegen grows the two opcodes it used to reject, and `rewriteBitExtensionInstrs` shrinks to what it was really for, turning the extensions that don't extend anything, a constant input or a source at least as wide as the destination, into plain Moves. That pass now has to return kUnchanged for a genuine extension, its stage runs to a fixpoint and reporting a change it did not make would spin forever. Marked a prototype: it builds and passes the runtime tests on x86-64, but the aarch64 paths are compile-tested only. Reviewed By: yoney Differential Revision: D116068880 fbshipit-source-id: 386eb98ce7f79f8fcc7543fbc94b763599e777eb
1 parent 7ff52db commit 14b5442

8 files changed

Lines changed: 43 additions & 58 deletions

File tree

‎cinderx/Jit/codegen/autogen.cpp‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2048,14 +2048,14 @@ void translateMovExtOp(
20482048
}
20492049
}
20502050

2051-
void translateMovZX(Environ* env, const Instruction* instr) {
2051+
void translateZext(Environ* env, const Instruction* instr) {
20522052
// ARM64 uxtb/uxth/ldrb/ldrh only accept W-register destinations.
20532053
// Writing to W implicitly zeros the upper 32 bits of the X register,
20542054
// so this correctly zero-extends to 64 bits even for k64bit outputs.
20552055
translateMovExtOp(
20562056
env,
20572057
instr,
2058-
"MovZX",
2058+
"Zext",
20592059
[](a64::Builder* as, auto output, auto input) {
20602060
as->uxtb(a64::w(output.id()), input);
20612061
},
@@ -2070,7 +2070,7 @@ void translateMovZX(Environ* env, const Instruction* instr) {
20702070
});
20712071
}
20722072

2073-
void translateMovSX(Environ* env, const Instruction* instr) {
2073+
void translateSext(Environ* env, const Instruction* instr) {
20742074
// The shared helper's 32-bit path only zero-extends, so sign-extending from
20752075
// 32 bits needs sxtw/ldrsw here.
20762076
const lir::Operand* input = instr->getInput(0);
@@ -2101,7 +2101,7 @@ void translateMovSX(Environ* env, const Instruction* instr) {
21012101
translateMovExtOp(
21022102
env,
21032103
instr,
2104-
"MovSX",
2104+
"Sext",
21052105
[](a64::Builder* as, auto... args) { as->sxtb(args...); },
21062106
[](a64::Builder* as, auto... args) { as->sxth(args...); },
21072107
[](a64::Builder* as, auto... args) { as->ldrsb(args...); },
@@ -2572,7 +2572,7 @@ void AutoTranslator::translateInstr(Environ* env, const Instruction* instr)
25722572
}
25732573
return;
25742574
}
2575-
case Opcode::kMovZX: {
2575+
case Opcode::kZext: {
25762576
auto* output = instr->output();
25772577
auto* input = instr->getInput(0);
25782578

@@ -2604,7 +2604,7 @@ void AutoTranslator::translateInstr(Environ* env, const Instruction* instr)
26042604
}
26052605
return;
26062606
}
2607-
case Opcode::kMovSX: {
2607+
case Opcode::kSext: {
26082608
auto* output = instr->output();
26092609
auto* input = instr->getInput(0);
26102610

@@ -3163,8 +3163,6 @@ void AutoTranslator::translateInstr(Environ* env, const Instruction* instr)
31633163
case Opcode::kNop:
31643164
case Opcode::kVectorCallTstate:
31653165
case Opcode::kVarArgCall:
3166-
case Opcode::kSext:
3167-
case Opcode::kZext:
31683166
case Opcode::kMulAdd:
31693167
case Opcode::kLoadArg:
31703168
case Opcode::kLoadSecondCallResult:
@@ -3189,11 +3187,11 @@ void AutoTranslator::translateInstr(Environ* env, const Instruction* instr)
31893187
case Opcode::kMoveRelaxed:
31903188
translateMove(env, instr);
31913189
return;
3192-
case Opcode::kMovZX:
3193-
translateMovZX(env, instr);
3190+
case Opcode::kZext:
3191+
translateZext(env, instr);
31943192
return;
3195-
case Opcode::kMovSX:
3196-
translateMovSX(env, instr);
3193+
case Opcode::kSext:
3194+
translateSext(env, instr);
31973195
return;
31983196
case Opcode::kUnreachable:
31993197
translateUnreachable(env, instr);
@@ -3437,8 +3435,6 @@ void AutoTranslator::translateInstr(Environ* env, const Instruction* instr)
34373435
case Opcode::kNop:
34383436
case Opcode::kVectorCallTstate:
34393437
case Opcode::kVarArgCall:
3440-
case Opcode::kSext:
3441-
case Opcode::kZext:
34423438
case Opcode::kLoadArg:
34433439
case Opcode::kLoadSecondCallResult:
34443440
case Opcode::kCondBranch:

‎cinderx/Jit/lir/generator.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,25 +1173,25 @@ void GenerateBoxedReturnWrapperBlocks(
11731173
auto arg0 = ARGUMENT_REGS[0];
11741174
if (return_type <= TCBool) {
11751175
box_block->allocateInstr(
1176-
Opcode::kMovZX,
1176+
Opcode::kZext,
11771177
nullptr,
11781178
OutPhyReg{arg0, DataType::k64bit},
11791179
PhyReg{ret8, DataType::k8bit});
11801180
} else if (return_type <= TCInt8 || return_type <= TCUInt8) {
11811181
box_block->allocateInstr(
1182-
return_type <= TCInt8 ? Opcode::kMovSX : Opcode::kMovZX,
1182+
return_type <= TCInt8 ? Opcode::kSext : Opcode::kZext,
11831183
nullptr,
11841184
OutPhyReg{arg0, DataType::k64bit},
11851185
PhyReg{ret8, DataType::k8bit});
11861186
} else if (return_type <= TCInt16 || return_type <= TCUInt16) {
11871187
box_block->allocateInstr(
1188-
return_type <= TCInt16 ? Opcode::kMovSX : Opcode::kMovZX,
1188+
return_type <= TCInt16 ? Opcode::kSext : Opcode::kZext,
11891189
nullptr,
11901190
OutPhyReg{arg0, DataType::k64bit},
11911191
PhyReg{ret16, DataType::k16bit});
11921192
} else if (return_type <= TCInt32) {
11931193
box_block->allocateInstr(
1194-
Opcode::kMovSX,
1194+
Opcode::kSext,
11951195
nullptr,
11961196
OutPhyReg{arg0, DataType::k64bit},
11971197
PhyReg{ret32, DataType::k32bit});
@@ -1204,7 +1204,7 @@ void GenerateBoxedReturnWrapperBlocks(
12041204
PY_VERSION_HEX < 0x030E0000 && kBuildArch == Arch::kAarch64;
12051205
if (arg0 != ret64 || needs_extend) {
12061206
box_block->allocateInstr(
1207-
Opcode::kMovZX,
1207+
Opcode::kZext,
12081208
nullptr,
12091209
OutPhyReg{arg0, DataType::k64bit},
12101210
PhyReg{ret32, DataType::k32bit});

‎cinderx/Jit/lir/ops.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,6 @@ bool writesFlags(Opcode opcode) {
233233
case Opcode::kLoadPair:
234234
case Opcode::kLoadSecondCallResult:
235235
case Opcode::kMovConstPool:
236-
case Opcode::kMovSX:
237-
case Opcode::kMovZX:
238236
case Opcode::kMove:
239237
case Opcode::kMoveRelaxed:
240238
case Opcode::kMulAdd:
@@ -312,8 +310,6 @@ bool isEssential(Opcode opcode) {
312310
case Opcode::kLoadSecondCallResult:
313311
case Opcode::kLoadThreadState:
314312
case Opcode::kMovConstPool:
315-
case Opcode::kMovSX:
316-
case Opcode::kMovZX:
317313
case Opcode::kMove:
318314
case Opcode::kMoveRelaxed:
319315
case Opcode::kMul:

‎cinderx/Jit/lir/ops.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,6 @@ namespace cinderx::jit::lir {
7575
X(LoadSecondCallResult) \
7676
X(LoadThreadState) \
7777
X(MovConstPool) \
78-
X(MovSX) \
79-
X(MovZX) \
8078
X(Move) \
8179
X(MoveRelaxed) \
8280
X(Mul) \

‎cinderx/Jit/lir/postalloc.cpp‎

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -523,14 +523,12 @@ RewriteResult rewriteCallInstrs(instr_iter_t instr_iter, Environ* env) {
523523
return kChanged;
524524
}
525525

526-
// Replaces ZEXT and SEXT with appropriate MOVE instructions.
526+
// Replaces the Zext and Sext instructions that aren't really extending
527+
// anything with plain Moves.
527528
RewriteResult rewriteBitExtensionInstrs(instr_iter_t instr_iter) {
528529
auto instr = instr_iter->get();
529530

530-
bool is_sext = instr->opcode() == Opcode::kSext;
531-
bool is_zext = instr->opcode() == Opcode::kZext;
532-
533-
if (!is_sext && !is_zext) {
531+
if (!instr->isSext() && !instr->isZext()) {
534532
return kUnchanged;
535533
}
536534

@@ -563,8 +561,7 @@ RewriteResult rewriteBitExtensionInstrs(instr_iter_t instr_iter) {
563561
case Operand::k8bit:
564562
case Operand::k16bit:
565563
case Operand::k32bit:
566-
instr->setOpcode(is_sext ? Opcode::kMovSX : Opcode::kMovZX);
567-
break;
564+
return kUnchanged;
568565
case Operand::k64bit:
569566
case Operand::kObject:
570567
case Operand::kObjectUntagged:
@@ -573,7 +570,7 @@ RewriteResult rewriteBitExtensionInstrs(instr_iter_t instr_iter) {
573570
JIT_ABORT("A float point number cannot be the input of the instruction.");
574571
}
575572

576-
return kChanged;
573+
JIT_ABORT("Unhandled input data type in '{}'", *instr);
577574
}
578575

579576
// Add (conditional) branch instructions to the end of each basic blocks when
@@ -636,7 +633,7 @@ RewriteResult rewriteBranchInstrs(Function* function) {
636633
RewriteResult optimizeMoveInstrs(instr_iter_t instr_iter) {
637634
auto instr = instr_iter->get();
638635
auto instr_opcode = instr->opcode();
639-
// Deliberately not MovZX/MovSX: a widening move still has to write the part
636+
// Deliberately not Sext/Zext: a widening move still has to write the part
640637
// of the destination that the source does not cover, even when the two name
641638
// the same register.
642639
if (instr_opcode != Opcode::kMove) {
@@ -1027,7 +1024,7 @@ RewriteResult rewriteSubWordRegMoves(instr_iter_t instr_iter) {
10271024

10281025
// After register allocation, spilled values become stack operands. ARM64 ALU
10291026
// instructions require register operands, so load stack inputs into scratch
1030-
// registers. Move/MovZX/MovSX/etc. natively support memory inputs and are
1027+
// registers. Move/Zext/Sext/etc. natively support memory inputs and are
10311028
// excluded.
10321029
RewriteResult rewriteMemoryInputsToReg(instr_iter_t instr_iter) {
10331030
auto instr = instr_iter->get();
@@ -1109,8 +1106,6 @@ RewriteResult rewriteMemoryInputsToReg(instr_iter_t instr_iter) {
11091106
case Opcode::kLoadSecondCallResult:
11101107
case Opcode::kLoadThreadState:
11111108
case Opcode::kMovConstPool:
1112-
case Opcode::kMovSX:
1113-
case Opcode::kMovZX:
11141109
case Opcode::kMove:
11151110
case Opcode::kMoveRelaxed:
11161111
case Opcode::kMulAdd:
@@ -1361,8 +1356,8 @@ RewriteResult rewriteDivide(instr_iter_t instr_iter) {
13611356
auto move = block->allocateInstrBefore(
13621357
instr_iter,
13631358
dividend_lower->isImm() ? Opcode::kMove
1364-
: instr->isDiv() ? Opcode::kMovSX
1365-
: Opcode::kMovZX,
1359+
: instr->isDiv() ? Opcode::kSext
1360+
: Opcode::kZext,
13661361
OutPhyReg(AX, DataType::k16bit));
13671362

13681363
if (dividend_lower->isImm()) {
@@ -1564,7 +1559,7 @@ RewriteResult optimizeMoveSequence(BasicBlock* basicblock) {
15641559
};
15651560

15661561
if (instr->isMove() || instr->isPush() || instr->isPop() ||
1567-
instr->isMovZX() || instr->isMovSX()) {
1562+
instr->isZext() || instr->isSext()) {
15681563
Operand* out = instr->output();
15691564
if (instr->isMove() && out->isStack() && instr->getInput(0)->isReg()) {
15701565
registerMemoryMoves.addRegisterToMemoryMove(

‎cinderx/Jit/lir/postgen.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -215,10 +215,10 @@ RewriteResult rewriteBinaryOpLargeConstant(instr_iter_t instr_iter) {
215215
// the first operand with a sign-extended version that matches the size of the
216216
// second operand.
217217
if (instr->getInput(0)->sizeInBits() < in1->sizeInBits()) {
218-
auto movsx = block->allocateInstrBefore(
219-
instr_iter, Opcode::kMovSX, OutVReg{in1->dataType()});
220-
movsx->appendInput(instr->releaseInput(0));
221-
instr->setInput(0, std::make_unique<Operand>(movsx, Operand::kLinked));
218+
auto sext = block->allocateInstrBefore(
219+
instr_iter, Opcode::kSext, OutVReg{in1->dataType()});
220+
sext->appendInput(instr->releaseInput(0));
221+
instr->setInput(0, std::make_unique<Operand>(sext, Operand::kLinked));
222222
}
223223

224224
// Replace the constant with the move.
@@ -518,7 +518,7 @@ RewriteResult rewriteLoadSecondCallResult(instr_iter_t instr_iter) {
518518
//
519519
// NOT handled here:
520520
// - Move/MoveRelaxed "Rm": IS the canonical load (the lowering target)
521-
// - MovZX/MovSX: specialized sign/zero-extending loads from stack
521+
// - Zext/Sext: specialized sign/zero-extending loads from stack
522522
// - Lea: takes the ADDRESS of a stack slot, not the value
523523
// - Call: late-created by PostRegAllocRewrite via setOpcode()
524524
// - Negate/Invert: handled by AArch64 target selection

‎cinderx/RuntimeTests/lir_abi_test.cpp‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -483,34 +483,34 @@ TEST_F(LIRABITest, TestkInvert_OutPhyReg_Mem) {
483483
}
484484
#endif
485485

486-
// kMovZX R r
487-
TEST_F(LIRABITest, TestkMovZX_OutPhyReg_PhyReg) {
486+
// kZext R r
487+
TEST_F(LIRABITest, TestkZext_OutPhyReg_PhyReg) {
488488
translateInstr(
489-
Opcode::kMovZX,
489+
Opcode::kZext,
490490
makeOutPhyReg(0, DataType::k64bit),
491491
makePhyReg(1, DataType::k32bit));
492492
}
493493

494-
// kMovZX R m
495-
TEST_F(LIRABITest, TestkMovZX_OutPhyReg_Mem) {
494+
// kZext R m
495+
TEST_F(LIRABITest, TestkZext_OutPhyReg_Mem) {
496496
translateInstr(
497-
Opcode::kMovZX,
497+
Opcode::kZext,
498498
makeOutPhyReg(0, DataType::k64bit),
499499
makeStk(-16, DataType::k32bit));
500500
}
501501

502-
// kMovSX R r
503-
TEST_F(LIRABITest, TestkMovSX_OutPhyReg_PhyReg) {
502+
// kSext R r
503+
TEST_F(LIRABITest, TestkSext_OutPhyReg_PhyReg) {
504504
translateInstr(
505-
Opcode::kMovSX,
505+
Opcode::kSext,
506506
makeOutPhyReg(0, DataType::k64bit),
507507
makePhyReg(1, DataType::k32bit));
508508
}
509509

510-
// kMovSX R m
511-
TEST_F(LIRABITest, TestkMovSX_OutPhyReg_Mem) {
510+
// kSext R m
511+
TEST_F(LIRABITest, TestkSext_OutPhyReg_Mem) {
512512
translateInstr(
513-
Opcode::kMovSX,
513+
Opcode::kSext,
514514
makeOutPhyReg(0, DataType::k64bit),
515515
makeStk(-16, DataType::k32bit));
516516
}

‎cinderx/RuntimeTests/lir_postalloc_test.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ TEST_F(LIRPostAllocRewriteTest, MoveSequenceLooksPastWideningMoves) {
258258
OutStk{kSlot, DataType::k64bit},
259259
PhyReg{kSpilled, DataType::k64bit});
260260
bb->allocateInstr(
261-
Opcode::kMovZX,
261+
Opcode::kZext,
262262
nullptr,
263263
OutPhyReg{kWidenOut, DataType::k64bit},
264264
PhyReg{kWidenIn, DataType::k32bit});

0 commit comments

Comments
 (0)