About issue 13836

[Yahan Lu]

After patch

```
plct-dev-7:~/source/node $ git diff HEAD^
diff --git a/deps/v8/src/builtins/builtins-definitions.h b/deps/v8/src/builtins/builtins-definitions.h
index c793ef521f..175acbd495 100644
--- a/deps/v8/src/builtins/builtins-definitions.h
+++ b/deps/v8/src/builtins/builtins-definitions.h
@@ -190,7 +190,7 @@ namespace internal {
  /* Baseline Compiler */                                                      \
  ASM(BaselineOutOfLinePrologue, BaselineOutOfLinePrologue)                    \
  ASM(BaselineOutOfLinePrologueDeopt, Void)                                    \
-  ASM(BaselineOnStackReplacement, OnStackReplacement)                  \

+  ASM(BaselineOnStackReplacement, BaselineOnStackReplacement)                          \
  ASM(BaselineLeaveFrame, BaselineLeaveFrame)                                  \
  ASM(BaselineOrInterpreterEnterAtBytecode, Void)                              \
  ASM(BaselineOrInterpreterEnterAtNextBytecode, Void)                          \

```

The riscv issue(https://bugs.chromium.org/p/v8/issues/detail?id=13836) be fixed. But in my understanding, OnStackReplacementDescriptor is the same as BaselineOnStackReplacementDescriptor.

refer:

1.https://chromium-review.googlesource.com/c/v8/v8/+/3859850/13/src/codegen/interface-descriptors-inl.h#373

[Yahan Lu]

The detailed commit

https://github.com/luyahan/nodejs-riscv64/commits/root-code

[Leszek Swirski]

The difference is that the old BaselineOnStackReplacementDescriptor was defined as having no context register (DEFINE_PARAMETERS_NO_CONTEXT vs DEFINE_PARAMETERS). It's not clear why this causes issues on riscv and not other architectures though...

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/f3512692-7d3e-4229-8e6d-8ccd0a3fc385n%40googlegroups.com.

[Leszek Swirski]

I think this is an unnecessary additional load, though I also wouldn't expect it to cause issues.

On Fri, Mar 31, 2023 at 3:08 PM Yahan Lu <ya...@iscas.ac.cn> wrote:

I have a question about builtins-arm64.cc?

void Builtins::Generate_InterpreterOnStackReplacement(MacroAssembler* masm) {
  using D = OnStackReplacementDescriptor;
  static_assert(D::kParameterCount == 1);
  OnStackReplacement(masm, OsrSourceTier::kInterpreter,
                     D::MaybeTargetCodeRegister());
}
void Builtins::Generate_BaselineOnStackReplacement(MacroAssembler* masm) {
  using D = OnStackReplacementDescriptor;
  static_assert(D::kParameterCount == 1);

  __ ldr(kContextRegister,
         MemOperand(fp, BaselineFrameConstants::kContextOffset));
  OnStackReplacement(masm, OsrSourceTier::kBaseline,
                     D::MaybeTargetCodeRegister());

}

InterpreterOnStackReplacement and BaselineOnStackReplacement has the same Descriptor. But why only load kContextRegister in BaselineOnStackReplacement?

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/5e7dcb7c-e0fc-4ad1-b79c-5e2b47d08e9fn%40googlegroups.com.