From 94f80d253c49a2b60148122da26c029cffb63fce Mon Sep 17 00:00:00 2001 From: Victor Gomes Date: Fri, 16 Dec 2022 10:58:00 +0100 Subject: [PATCH] [maglev] Avoid copying temporary registers to deferred code ... where scratch scope is no longer active and the registers could be clobbered. Bug: v8:7700 Change-Id: I366312e2cafd732199b4668fd5f40363688c8a04 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4107089 Reviewed-by: Leszek Swirski Commit-Queue: Leszek Swirski Commit-Queue: Victor Gomes Auto-Submit: Victor Gomes Cr-Commit-Position: refs/heads/main@{#84897} --- src/codegen/arm64/register-arm64.h | 7 +++ src/maglev/arm64/maglev-assembler-arm64.cc | 6 ++- src/maglev/arm64/maglev-ir-arm64.cc | 59 +++++++++------------- src/maglev/maglev-assembler.h | 30 +++++++++-- 4 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/codegen/arm64/register-arm64.h b/src/codegen/arm64/register-arm64.h index 728ac559d5..f027a8500a 100644 --- a/src/codegen/arm64/register-arm64.h +++ b/src/codegen/arm64/register-arm64.h @@ -45,6 +45,9 @@ namespace internal { ALWAYS_ALLOCATABLE_GENERAL_REGISTERS(V) \ MAYBE_ALLOCATABLE_GENERAL_REGISTERS(V) +#define MAGLEV_SCRATCH_GENERAL_REGISTERS(R) \ + R(x16) R(x17) + #define FLOAT_REGISTERS(V) \ V(s0) V(s1) V(s2) V(s3) V(s4) V(s5) V(s6) V(s7) \ V(s8) V(s9) V(s10) V(s11) V(s12) V(s13) V(s14) V(s15) \ @@ -76,6 +79,10 @@ namespace internal { R(d8) R(d9) R(d10) R(d11) R(d12) R(d13) R(d14) R(d16) \ R(d17) R(d18) R(d19) R(d20) R(d21) R(d22) R(d23) R(d24) \ R(d25) R(d26) R(d27) R(d28) + +#define MAGLEV_SCRATCH_DOUBLE_REGISTERS(R) \ + R(d30) R(d31) + // clang-format on // Some CPURegister methods can return Register and VRegister types, so we diff --git a/src/maglev/arm64/maglev-assembler-arm64.cc b/src/maglev/arm64/maglev-assembler-arm64.cc index ccf80aadd9..6f2cd45c9b 100644 --- a/src/maglev/arm64/maglev-assembler-arm64.cc +++ b/src/maglev/arm64/maglev-assembler-arm64.cc @@ -199,14 +199,16 @@ void MaglevAssembler::Prologue(Graph* graph) { AssertFeedbackVector(feedback_vector, flags); DeferredCodeInfo* deferred_flags_need_processing = PushDeferredCode( - [](MaglevAssembler* masm, Register flags, Register feedback_vector) { + [](MaglevAssembler* masm, Register feedback_vector) { ASM_CODE_COMMENT_STRING(masm, "Optimized marker check"); // TODO(leszeks): This could definitely be a builtin that we // tail-call. + UseScratchRegisterScope temps(masm); + Register flags = temps.AcquireX(); __ OptimizeCodeOrTailCallOptimizedCodeSlot(flags, feedback_vector); __ Trap(); }, - flags, feedback_vector); + feedback_vector); LoadFeedbackVectorFlagsAndJumpIfNeedsProcessing( flags, feedback_vector, CodeKind::MAGLEV, diff --git a/src/maglev/arm64/maglev-ir-arm64.cc b/src/maglev/arm64/maglev-ir-arm64.cc index 49d95626ff..3a8a68dc9d 100644 --- a/src/maglev/arm64/maglev-ir-arm64.cc +++ b/src/maglev/arm64/maglev-ir-arm64.cc @@ -497,10 +497,9 @@ void Int32DivideWithOverflow::GenerateCode(MaglevAssembler* masm, } void Int32ModulusWithOverflow::SetValueLocationConstraints() { - UseRegister(left_input()); - UseRegister(right_input()); + UseAndClobberRegister(left_input()); + UseAndClobberRegister(right_input()); DefineAsRegister(this); - set_temporaries_needed(1); } void Int32ModulusWithOverflow::GenerateCode(MaglevAssembler* masm, const ProcessingState& state) { @@ -521,16 +520,11 @@ void Int32ModulusWithOverflow::GenerateCode(MaglevAssembler* masm, // lhs % rhs Register left = ToRegister(left_input()).W(); - Register right_maybe_neg = ToRegister(right_input()).W(); + Register right = ToRegister(right_input()).W(); Register out = ToRegister(result()).W(); ZoneLabelRef done(masm); ZoneLabelRef rhs_checked(masm); - - UseScratchRegisterScope temps(masm); - Register right = temps.AcquireW(); - __ Move(right, right_maybe_neg); - __ Cmp(right, Immediate(0)); __ JumpToDeferredIf( le, @@ -544,30 +538,26 @@ void Int32ModulusWithOverflow::GenerateCode(MaglevAssembler* masm, __ bind(*rhs_checked); __ Cmp(left, Immediate(0)); - { - UseScratchRegisterScope temps(masm); - Register scratch = temps.AcquireW(); - __ JumpToDeferredIf( - lt, - [](MaglevAssembler* masm, ZoneLabelRef done, Register left_neg, - Register right, Register out, Register scratch, - Int32ModulusWithOverflow* node) { - Register left = scratch; - Register res = node->general_temporaries().first().W(); - __ neg(left, left_neg); - __ udiv(res, left, right); - __ msub(out, res, right, left); - __ cmp(out, Immediate(0)); - // TODO(victorgomes): This ideally should be kMinusZero, but Maglev - // only allows one deopt reason per IR. - __ EmitEagerDeoptIf(eq, DeoptimizeReason::kDivisionByZero, node); - __ neg(out, out); - __ b(*done); - }, - done, left, right, out, scratch, this); - } + __ JumpToDeferredIf( + lt, + [](MaglevAssembler* masm, ZoneLabelRef done, Register left, + Register right, Register out, Int32ModulusWithOverflow* node) { + UseScratchRegisterScope temps(masm); + Register res = temps.AcquireW(); + __ neg(left, left); + __ udiv(res, left, right); + __ msub(out, res, right, left); + __ cmp(out, Immediate(0)); + // TODO(victorgomes): This ideally should be kMinusZero, but Maglev + // only allows one deopt reason per IR. + __ EmitEagerDeoptIf(eq, DeoptimizeReason::kDivisionByZero, node); + __ neg(out, out); + __ b(*done); + }, + done, left, right, out, this); Label right_not_power_of_2; + UseScratchRegisterScope temps(masm); Register mask = temps.AcquireW(); __ Add(mask, right, Immediate(-1)); __ Tst(mask, right); @@ -817,6 +807,7 @@ void CheckInt32IsSmi::GenerateCode(MaglevAssembler* masm, void CheckedSmiTagInt32::SetValueLocationConstraints() { UseRegister(input()); DefineAsRegister(this); + set_temporaries_needed(1); } void CheckedSmiTagInt32::GenerateCode(MaglevAssembler* masm, const ProcessingState& state) { @@ -833,11 +824,11 @@ void CheckedSmiTagInt32::GenerateCode(MaglevAssembler* masm, void CheckedInternalizedString::SetValueLocationConstraints() { UseRegister(object_input()); DefineSameAsFirst(this); + set_temporaries_needed(1); } void CheckedInternalizedString::GenerateCode(MaglevAssembler* masm, const ProcessingState& state) { - UseScratchRegisterScope temps(masm); - Register scratch = temps.AcquireX(); + Register scratch = general_temporaries().PopFirst(); Register object = ToRegister(object_input()); if (check_type_ == CheckType::kOmitHeapObjectCheck) { @@ -850,7 +841,7 @@ void CheckedInternalizedString::GenerateCode(MaglevAssembler* masm, __ LoadMap(scratch, object); __ RecordComment("Test IsInternalizedString"); // Go to the slow path if this is a non-string, or a non-internalised string. - __ Ldrh(scratch, FieldMemOperand(scratch, Map::kInstanceTypeOffset)); + __ Ldr(scratch, FieldMemOperand(scratch, Map::kInstanceTypeOffset)); __ Tst(scratch, Immediate(kIsNotStringMask | kIsNotInternalizedMask)); static_assert((kStringTag | kInternalizedTag) == 0); ZoneLabelRef done(masm); diff --git a/src/maglev/maglev-assembler.h b/src/maglev/maglev-assembler.h index 554d1d67f9..2f678286e5 100644 --- a/src/maglev/maglev-assembler.h +++ b/src/maglev/maglev-assembler.h @@ -271,6 +271,15 @@ struct CopyForDeferredByValue { } }; +#ifdef V8_TARGET_ARCH_ARM64 +#define LIST_REG(V) V, +static constexpr RegList kScratchGeneralRegisters = { + MAGLEV_SCRATCH_GENERAL_REGISTERS(LIST_REG) Register::no_reg()}; +static constexpr DoubleRegList kScratchDoubleRegisters = { + MAGLEV_SCRATCH_DOUBLE_REGISTERS(LIST_REG) DoubleRegister::no_reg()}; +#undef LIST_REG +#endif // V8_TARGET_ARCH_ARM64 + // Node pointers are copied by value. template struct CopyForDeferredHelper< @@ -291,11 +300,24 @@ struct CopyForDeferredHelper : public CopyForDeferredByValue {}; // Machine registers are copied by value. template <> -struct CopyForDeferredHelper - : public CopyForDeferredByValue {}; +struct CopyForDeferredHelper { + static Register Copy(MaglevCompilationInfo* compilation_info, Register reg) { +#ifdef V8_TARGET_ARCH_ARM64 + DCHECK(!kScratchGeneralRegisters.has(reg)); +#endif // V8_TARGET_ARCH_ARM64 + return reg; + } +}; template <> -struct CopyForDeferredHelper - : public CopyForDeferredByValue {}; +struct CopyForDeferredHelper { + static DoubleRegister Copy(MaglevCompilationInfo* compilation_info, + DoubleRegister reg) { +#ifdef V8_TARGET_ARCH_ARM64 + DCHECK(!kScratchDoubleRegisters.has(reg)); +#endif // V8_TARGET_ARCH_ARM64 + return reg; + } +}; // Bytecode offsets are copied by value. template <> struct CopyForDeferredHelper