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

Work towards RISC-V cross-compilation support #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fwsGonzo
Copy link

@fwsGonzo fwsGonzo commented Jul 9, 2024

This is probably not going to be a mergable PR, but I imagine when RISC-V support is added, this work will help. I'm using a RISC-V emulator to run build-generators instead of complicating the CMake script

@fwsGonzo fwsGonzo marked this pull request as draft July 9, 2024 17:32
@fwsGonzo fwsGonzo force-pushed the riscv branch 3 times, most recently from bde3e50 to 8d70c01 Compare July 9, 2024 18:35
@fwsGonzo
Copy link
Author

fwsGonzo commented Jul 10, 2024

After some testing it seems that everything works, so I'm undrafting this.

For building this on actual 64-bit RISC-V there are unnecessary changes:

  • No RISC-V emulator is needed to run build-gen programs, like mksnapshot
  • No need for -static or for the occasional -latomic work-around
  • Including v8config.h in v8/src/heap/base/asm/riscv/push_registers_asm.cc is perhaps still necessary, but perhaps better done from CMake?

@fwsGonzo fwsGonzo marked this pull request as ready for review July 10, 2024 10:18
@@ -79,14 +78,23 @@ if (CMAKE_SYSTEM_PROCESSOR MATCHES "^(ppc|powerpc)")
else()
set(is-ppc 1)
endif()
endif ()
endif ()
Copy link
Owner

Choose a reason for hiding this comment

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

This is an accidental whitespace change, isn't it? If so:

Suggested change
endif ()
endif ()

@@ -305,6 +313,7 @@ else()
$<${is-ppc}:v8/src/heap/base/asm/ppc/push_registers_asm.cc>
$<${is-s390}:v8/src/heap/base/asm/s390/push_registers_asm.cc>
$<${is-x64}:v8/src/heap/base/asm/x64/push_registers_asm.cc>
$<${is-riscv}:v8/src/heap/base/asm/riscv/push_registers_asm.cc>
Copy link
Owner

Choose a reason for hiding this comment

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

s/tab/whitespace/

Suggested change
$<${is-riscv}:v8/src/heap/base/asm/riscv/push_registers_asm.cc>
$<${is-riscv}:v8/src/heap/base/asm/riscv/push_registers_asm.cc>

@@ -625,16 +660,21 @@ target_link_libraries(v8_snapshot
v8-bytecodes-builtin-list
)

if (is-riscv)
set(mksnp-prefix ${RISCV_EMULATOR} -P --)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
set(mksnp-prefix ${RISCV_EMULATOR} -P --)
set(mksnp-prefix ${RISCV_EMULATOR} -P --)

And consider naming it mksnapshot-prefix for clarity.

@@ -1005,9 +1051,13 @@ target_include_directories(v8_torque_generated
file(WRITE "${PROJECT_BINARY_DIR}/touch_torque_outputs.cmake"
"file(TOUCH ${torque-outputs};${torque_outputs})")

if(is-riscv)
set(torque-prefix ${RISCV_EMULATOR} -P --) # Proxy through RISC-V emulator
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
set(torque-prefix ${RISCV_EMULATOR} -P --) # Proxy through RISC-V emulator
set(torque-prefix ${RISCV_EMULATOR} -P --) # Proxy through RISC-V emulator

@@ -1073,6 +1123,7 @@ target_link_libraries(mksnapshot
v8_initializers
v8-bytecodes-builtin-list
v8_torque_generated
-static
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
-static
-static

I half and half expect this to break Windows builds though.

@@ -1,3 +1,7 @@
if (is-riscv)
set(bblg-prefix ${RISCV_EMULATOR} -P --) # Proxy through the emulator
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
set(bblg-prefix ${RISCV_EMULATOR} -P --) # Proxy through the emulator
set(bblg-prefix ${RISCV_EMULATOR} -P --) # Proxy through the emulator

And please name it less abbrev.

@@ -90,4 +91,6 @@ asm(".global PushAllRegistersAndIterateStack \n"
" lw s0, 0(sp) \n"
" addi sp, sp, 56 \n"
" jr ra \n");
#else
#error "RISC-V PushAllRegistersAndIterateStack unable to determine target arch."
Copy link
Owner

Choose a reason for hiding this comment

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

Patches to V8 should go into patches/, not be directly applied, otherwise the next update will blow them away.

But! - and this is important - changes should be submitted to upstream first, because I don't want to be forward-porting patches forever if I can avoid it. See https://v8.dev/docs/contribute for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants