Skip to content

Conversation

@no1wudi
Copy link
Collaborator

@no1wudi no1wudi commented Jun 7, 2021

No description provided.

@no1wudi no1wudi marked this pull request as draft June 7, 2021 06:22
@no1wudi
Copy link
Collaborator Author

no1wudi commented Jun 7, 2021

Now native code generation seem works by emit native object file, we still need to implement relocation logic for riscv platform.

@wenyongh
Copy link
Contributor

wenyongh commented Jun 7, 2021

Now native code generation seem works by emit native object file, we still need to implement relocation logic for riscv platform.

Yes, so could you please add file aot_reloc_riscv32.c and aot_reloc_riscv64.c under core/iwasm/aot/arch/ to implement the relocation? If some relocation types are not easy to implement, we can work together to resolve them.

@no1wudi
Copy link
Collaborator Author

no1wudi commented Jun 7, 2021

Yes, so could you please add file aot_reloc_riscv32.c and aot_reloc_riscv64.c under core/iwasm/aot/arch/ to implement the relocation? If some relocation types are not easy to implement, we can work together to resolve them.

@wenyongh OK, and we can merge the invokeNative riscv32/riscv64 version into one since they share the common ISA except width of register like this: https://github.com/zephyrproject-rtos/zephyr/blob/1f0b783bbb2cd0546d0fee12d72cccc694847915/include/arch/riscv/arch.h#L190

@wenyongh
Copy link
Contributor

wenyongh commented Jun 7, 2021

Yes, so could you please add file aot_reloc_riscv32.c and aot_reloc_riscv64.c under core/iwasm/aot/arch/ to implement the relocation? If some relocation types are not easy to implement, we can work together to resolve them.

@wenyongh OK, and we can merge the invokeNative riscv32/riscv64 version into one since they share the common ISA except width of register like this: https://github.com/zephyrproject-rtos/zephyr/blob/1f0b783bbb2cd0546d0fee12d72cccc694847915/include/arch/riscv/arch.h#L190

Should be aot_reloc_riscv.c? OK, it is good to use only one file.

@no1wudi
Copy link
Collaborator Author

no1wudi commented Jun 7, 2021

Should be aot_reloc_riscv.c? OK, it is good to use only one file.

Yes, and we should merge these files into invokeNattive_riscv.s:

  • invokeNative_riscv32_ilp32.s
  • invokeNative_riscv32_ilp32d.s
  • invokeNative_riscv32_ilp64.s
  • invokeNative_riscv32_ilp32d.s

@wenyongh I've add a new file invokeNative_riscv.S, tested with rv32 imac, could you do some tests on other risc v platform? Then we can remove files above.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

Should remove the original .s files and change the related make files?

@no1wudi no1wudi force-pushed the main branch 5 times, most recently from 2d060fb to 2ec71ea Compare June 8, 2021 02:21
@no1wudi no1wudi requested a review from wenyongh June 9, 2021 02:44
@no1wudi no1wudi marked this pull request as ready for review June 16, 2021 09:57
switch (reloc_type) {
case R_RISCV_CALL:
/* lui t0, hi20 */
*(uint32 *)(P + 0) = (uint32)hi20 | 0x2b7;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Potential bugs here since we use absolute address to jump, it works fine on rv32, but on rv64 platform, target address may exceed the 32bit (4GB) address space.

Please take a look @wenyongh

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, should add check, like R_X86_64_PC32/R_X86_64_32/R_X86_64_32S in aot_reloc_x86_64.c. Seems that here we should check whether (uint32)(uintptr_t)symbol_addr == (uintptr_t)symbol_addr?

If exceeds 4G, had better prompt using --size-level=1 if it fixes the issue.
And in aot_loader.c, try adding MMAP_MAP_32BIT for mmap:

#if defined(BUILD_TARGET_X86_64) || defined(BUILD_TARGET_AMD_64) \
     || defined(BUILD_TARGET_RISCV64)
        /* aot code and data in x86_64 must be in range 0 to 2G due to
           relocation for R_X86_64_32/32S/PC32 */
        int map_flags = MMAP_MAP_32BIT;
#else
        int map_flags = MMAP_MAP_NONE;
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Not sure whether there is only one relocation type needed to implement. Do you want to merge to patch firstly, and then test the spec cases and some standalone cases, or test the spec cases firstly, implement all the relocation types and then merge the patch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to implement all relocation types step by step since R_RISCV_CALL meet the requirements of our current workload.

Will this block the next release?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is OK, we had better test more cases, not sure whether your riscv hardware can run Linux system? If not, you can enable the QEMU to test them, ref to Fedora QEMU, and run it on Linux, the ABI is RISCV64_LP64D.

switch (reloc_type) {
case R_RISCV_CALL:
/* lui t0, hi20 */
*(uint32 *)(P + 0) = (uint32)hi20 | 0x2b7;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, should add check, like R_X86_64_PC32/R_X86_64_32/R_X86_64_32S in aot_reloc_x86_64.c. Seems that here we should check whether (uint32)(uintptr_t)symbol_addr == (uintptr_t)symbol_addr?

If exceeds 4G, had better prompt using --size-level=1 if it fixes the issue.
And in aot_loader.c, try adding MMAP_MAP_32BIT for mmap:

#if defined(BUILD_TARGET_X86_64) || defined(BUILD_TARGET_AMD_64) \
     || defined(BUILD_TARGET_RISCV64)
        /* aot code and data in x86_64 must be in range 0 to 2G due to
           relocation for R_X86_64_32/32S/PC32 */
        int map_flags = MMAP_MAP_32BIT;
#else
        int map_flags = MMAP_MAP_NONE;
#endif

switch (reloc_type) {
case R_RISCV_CALL:
/* lui t0, hi20 */
*(uint32 *)(P + 0) = (uint32)hi20 | 0x2b7;
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Not sure whether there is only one relocation type needed to implement. Do you want to merge to patch firstly, and then test the spec cases and some standalone cases, or test the spec cases firstly, implement all the relocation types and then merge the patch?

@no1wudi no1wudi force-pushed the main branch 2 times, most recently from 415254e to deae428 Compare June 17, 2021 07:10
@no1wudi no1wudi requested a review from wenyongh June 17, 2021 09:10

case R_RISCV_HI20:
{
long offset = (long)symbol_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be (long)(symbol_addr + reloc_addend); //S + A

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, shall we check whether S + A in range 0 - 2^20-1?


case R_RISCV_LO12_I:
{
long offset = (long)symbol_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, (long)(symbol_addr + reloc_addend), and check whether S + A is in range 0 - 2^12-1?


case R_RISCV_LO12_S:
{
long offset = (long)symbol_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, (long)(symbol_addr + reloc_addend), and check whether S + A is in range 0 - 2^12-1?

no1wudi and others added 5 commits July 19, 2021 16:31
Implement more relocation types, re-org code and fix some issues
Merge pull request #3 from wenyongh/test/riscv_aot
Enable os_mmap with MAP_32BIT flag for riscv64
@wenyongh wenyongh merged commit e4023c8 into bytecodealliance:main Jul 22, 2021
wenyongh referenced this pull request in wenyongh/wasm-micro-runtime Jul 22, 2021
Implement AOT support for RISCV (#649)
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Enable RISCV AOT support, the supported ABIs are LP64 and LP64D for riscv64, ILP32 and ILP32D for riscv32.
For wamrc:
    use --target=riscv64/riscv32 to specify the target arch of output AOT file,
    use --target-abi=lp64d/lp64/ilp32d/ilp32 to specify the target ABI,
    if --target-abi isn't specified, by default lp64d is used for riscv64, and ilp32d is used for riscv32.

Signed-off-by: Huang Qi <[email protected]>
Co-authored-by: wenyongh <[email protected]>
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.

3 participants