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

cpu: risc-v: add RISC-V defines and fix indentation #1148

Merged
merged 1 commit into from
Sep 14, 2021

Conversation

aaronfranke
Copy link
Contributor

@aaronfranke aaronfranke commented Sep 2, 2021

Description

This PR does 2 things:

  • Fix indentation in cmake/platform.cmake, part of the file was indented by 5 spaces instead of 4.
  • Add RISC-V defines. See Add support for the RISC-V architecture #1146 for context. oneDNN does successfully compile both with and without these changes, but now it's possible to explicitly target RV64 (useful for cross-compiling for example), and it's also possible to specify which specific extensions you're targeting (if someone wants something different from the default, editing the cmake file is required). If trying to do so from a machine that can't compile for it, this message will show: cc1plus: error: bad value (‘rv64gc’) for ‘-march=’ switch, which is nice.

Closes #1146. I did not make an RFC for this, I'm not sure that it's necessary for such a small change.

Checklist

General

  • Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit?
    • EDIT: They seem to pass, but didn't at first...? Anyway I'll leave the original text below:
    • I ran make test on RISC-V and most tests pass but these two tests fail:
        Start  69: test_lrn_backward
 69/108 Test  #69: test_lrn_backward .................................***Failed  161.46 sec
        Start  70: test_lrn_forward
 70/108 Test  #70: test_lrn_forward ..................................***Failed   35.99 sec
98% tests passed, 2 tests failed out of 108

Total Test time (real) = 6187.22 sec

The following tests FAILED:
         69 - test_lrn_backward (Failed)
         70 - test_lrn_forward (Failed)

I don't know how to debug the test. Also, the very long time is expected because this was ran in an emulator.

  • Have you formatted the code using clang-format?
    • Yes, I ran clang-format src/cpu/platform.hpp > src/cpu/platform.hpp2 then copied from hpp2 to hpp.

New features

  • Have you published an RFC for the new feature? No, not sure if it's necessary for this.
  • Was the RFC approved? N/A
  • Have you added relevant tests? N/A

@igorsafo
Copy link
Contributor

igorsafo commented Sep 2, 2021

Hello @aaronfranke ,
Thanks for the PR!
The changes look good to me.
Could you please share tools/environment you used to emulate RISC-V?

Usually if test is indicated as failed in summary it prints errors during the execution above so you can find the reason of a failure there.

Regards,
Igor Safonov

CMakeLists.txt Show resolved Hide resolved
src/cpu/README.md Show resolved Hide resolved
@igorsafo igorsafo self-assigned this Sep 2, 2021
@vpirogov
Copy link
Member

vpirogov commented Sep 2, 2021

If you want these changes to land in oneDNN v2.4 please backport to rls-v2.4 once the PR lands into master.

@vpirogov vpirogov added this to the v2.5 milestone Sep 3, 2021
@aaronfranke
Copy link
Contributor Author

aaronfranke commented Sep 3, 2021

@igorsafo Here are some instructions based on what I did. It's roughly based on https://wiki.ubuntu.com/RISC-V but note that this documentation is a bit out of date.

  1. Have Ubuntu 20.04 host and download the Ubuntu 20.04 "riscv64" image or have an Ubuntu 21.04 host and download the Ubuntu 21.04 "riscv64" image (this software is really picky about the versions matching).
  2. Install the dependencies: sudo apt install qemu-system-misc opensbi u-boot-qemu qemu-utils
  3. Optionally rename the image to something simpler.
  4. Extract the image from step 0: xz -dk ubuntu.img.xz
  5. Increase the size of the image qemu-img resize -f raw ubuntu.img +20G
    • Be aware, this can technically be changed later but it's really annoying to do, I recommend setting the size now to whatever size you think you'll need, I ended up running out of space and spent over an hour fiddling with fdisk and such.
  6. Make a Bash script with this command (or just run it) to boot the VM:
qemu-system-riscv64 \
-machine virt \
-nographic \
-m 8192 -smp 4 \
-bios /usr/lib/riscv64-linux-gnu/opensbi/generic/fw_jump.elf \
-kernel /usr/lib/u-boot/qemu-riscv64_smode/uboot.elf \
-device virtio-net-pci,netdev=eth0 \
-netdev user,id=eth0,hostfwd=tcp::2222-:22 \
-drive file=ubuntu.img,format=raw,if=virtio \
  1. Let it boot, this may take several minutes. Then log in.
  2. Do not run sudo apt upgrade as changing the kernel or bootloader can brick the VM. Again, it's really picky/sensitive.
  3. By default it uses the serial port, but this is quite bad. Run sudo apt install openssh-server, open a terminal on your host, and run ssh -p 2222 ubuntu@localhost to SSH into your VM for a much better experience.
  4. Install whatever you want as long as you don't run upgrade. Like sudo apt install cmake gcc clang build-essential
  5. Clone oneDNN and compile it. Or anything else for that matter.

@igorsafo
Copy link
Contributor

@aaronfranke Unfortunately I wasn't able to run ubuntu under qemu targeting riscv cpu. Could you please provide more details about the issues you see on riscv?

@aaronfranke
Copy link
Contributor Author

@igorsafo The failed tests did not give me any debug information other than what I posted above. If you would like, I can re-run the tests if you provide information of what arguments/flags/etc to use to show us more debug information.

That said, fixing those two tests is necessarily not a blocker to get this merged. 98% of tests passing on RISC-V is better than not having any RISC-V support.

@igorsafo
Copy link
Contributor

@aaronfranke To get more details about test issues you need to run the failing tests manually:

$ DNNL_VERBOSE=1 ./build/tests/gtests/test_lrn_forward
$ DNNL_VERBOSE=1 ./build/tests/gtests/test_lrn_backward

The tests will print cases that failed and a root cause. DNNL_VERBOSE will print a particular primitive implementation which was used for each test case.

I think it is ok to promote RISC-V support if it provides value to oneDNN users even if there are minor issues related to particular implementations, which can be addressed later.

@aaronfranke
Copy link
Contributor Author

test_lrn_forward.txt

test_lrn_backward.txt

Hmm, weird, now they say pass. I'm not sure if there is anything to do here.

@aaronfranke aaronfranke changed the title Add RISC-V defines and fix indentation cpu: risc-v: add RISC-V defines and fix indentation Sep 13, 2021
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.

Add support for the RISC-V architecture
4 participants