targets.py: Add ABIName parameter for RISC-V hard float targets #775

occheung · 2021-10-12T05:04:46Z

LLVM requires RISC-V targets to specify ABI if hard float is supported. The current implementation does not allow TargetOptions.McOptions.ABIName to be specified.

This patch exposes the ABIName to the create_target_machine function. So a RISC-V target that supports hard-float can be created like such:

        lltarget = llvm.Target.from_triple("riscv32-unknown-linux")
        llmachine = lltarget.create_target_machine(
                        features="+m,+a,+f,+d",
                        reloc="pic", codemodel="default", abiname="ilp32d")

Similar feature was also added to Rust's LLVM interface: See discussion | See commit

Alternatively, we can also process the target triple and features to deduce the appropriate ABI.

esc · 2021-10-12T07:45:45Z

@occheung thank you for contributing this. I have added it to the queue for review.

sbourdeauducq · 2021-11-08T08:51:10Z

Can we get this merged? Sounds like a simple and valid change.

esc · 2021-11-08T10:35:53Z

Can we get this merged? Sounds like a simple and valid change.

@sbourdeauducq I think the problem is that access to RISC-V is limited. Have you been able to test this on some real hardware?

gmarkall · 2021-11-08T12:08:27Z

I can test this - will post an update once I've done so.

sbourdeauducq · 2021-11-08T12:50:57Z

Have you been able to test this on some real hardware?

Definitely - our compiler uses llvmlite with the patch and we test it on our FPGA board, we even have hardware-in-the-loop CI.

gmarkall · 2021-11-15T10:00:30Z

@sbourdeauducq How are you building and testing this? Are you building an llvmlite with the RISCV target enabled?

sbourdeauducq · 2021-11-15T10:43:52Z

If you meant LLVM with the RISCV target enabled - yes.
Then we have a FPGA board with MiSoC and VexRiscv.

gmarkall · 2021-11-15T10:46:46Z

Thanks... I was originally going about testing this the wrong way - I was building on a RISC-V host and trying to find a way to get the LLVM JIT to work so the whole test suite could run - I've not completed testing this PR yet but haven't forgotten it.

sbourdeauducq · 2021-11-15T10:47:47Z

If you don't have any hardware to test, QEMU can emulate RISC-V. Or just looking at the generated assembly should be enough anyway.

gmarkall · 2021-11-15T10:52:26Z

In that case, is it possible to add a test that can run on the host with a RISC-V target that can check for the expected output in the generated assembly? That would make it a lot easier to check that I'm checking this PR correctly (and also lay a good foundation for future RISC-V-specific code generation functionality and tests).

seibert · 2021-11-15T16:03:42Z

#785 is related, as we will want to add RISCV as a target in our LLVM builds in order to run any RISC-V tests in our CI systems.

occheung · 2021-11-16T04:51:43Z

In that case, is it possible to add a test that can run on the host with a RISC-V target that can check for the expected output in the generated assembly?

Tests regarding the impact of ABI are performed in LLVM already, such as this (albeit in later LLVM version).

AFAIK the ABIs should have no influence on how the LLVM IR needs be written?

sbourdeauducq · 2021-11-16T04:53:06Z

AFAIK the ABIs should have no influence on how the LLVM IR needs be written?

Yes.
The only thing that can be tested here is that when the ABI option is set from the high-level Python code, the assembly (machine, not IR) code generated at the end by LLVM is affected appropriately.

occheung · 2021-11-16T09:15:08Z

Added test to show difference between ABIs. Tests passed locally. Do I enable RISC-V target in LLVM? (Is it through this file?)

gmarkall · 2021-11-16T09:29:20Z

Many thanks for adding the tests! I think that you have found the correct location to modify, but I'm not sure if there's additional places it would need modifying too - I will check.

gmarkall · 2021-11-16T09:31:08Z

There's also https://github.com/numba/llvmlite/blob/master/conda-recipes/llvmdev/bld.bat#L10 and https://github.com/numba/llvmlite/blob/master/conda-recipes/llvmdev/build.sh#L8 to modify as well.

gmarkall · 2021-11-16T09:54:00Z

Many thanks for adding the RISCV target - note that this will need the llvmdev / llvmlite packages to be rebuilt on our channel before the test starts to pass in CI.

esc · 2021-11-16T10:02:05Z

@gmarkall thank you for tending to this PR. We will need to decide what llvmlite version this should be a part of.

sbourdeauducq · 2021-11-16T10:08:33Z

From our perspective we would really appreciate a Conda Win64 package with RISC-V, this, and #702.
It's easy to have patches on our Hydra CI server and NixOS machines, but for Windows it's a very different story.

gmarkall

This generally looks good to me. Some notes:

WIth Add RISC-V + all default targets to LLVM build #788, the tests passed. 🎉
I was a little concerned about fragility of the test because it checks the assembly strings line-for-line, but on reflection I don't think they are likely to change unless we upgrade LLVM, so they will likely be stable enough in their current form.
The documentation in docs/source/user-guide/binding/target-information.rst will also need updating, as it isn't generated from the docstring.

gmarkall · 2021-11-22T10:46:34Z

@occheung Many thanks for updating the docs!

A side note: could I please ask that for future changes you merge master rather than rebasing please? The reason for this is that Github doesn't make it easy to track what happened on the reviewing side in newly-added commits. I think the only substantive change in your last push is the addition of the docs, but it's quite hard for me to tell for certain. Many thanks in advance!

gmarkall

This looks good to me, assuming that CI passes after new packages have been built following the merge of #788.

gmarkall · 2021-11-23T10:27:23Z

/azp run

azure-pipelines · 2021-11-23T10:27:28Z

Commenter does not have sufficient privileges for PR 775 in repo numba/llvmlite

esc · 2021-11-23T10:31:10Z

/azp run

azure-pipelines · 2021-11-23T10:31:15Z

Pull request contains merge conflicts.

esc · 2021-11-23T12:18:47Z

@gmarkall I have pushed the conflict resolution patch.

esc · 2021-11-23T12:58:58Z

@gmarkall it looks like this is still bust

gmarkall · 2021-11-23T14:05:50Z

@esc Let's discuss this a little in the dev meeting if there's time.

stuartarchibald · 2021-11-23T14:38:53Z

Is the issue that the build scripts are looking in channel numba and picking up build 3:
https://github.com/numba/llvmlite/blob/master/buildscripts/incremental/setup_conda_environment.sh#L30
Log extract:

2021-11-23T12:23:01.4828781Z   added / updated specs:
2021-11-23T12:23:01.4830437Z     - llvmdev=11
2021-11-23T12:23:01.4830913Z 
2021-11-23T12:23:01.4831264Z 
2021-11-23T12:23:01.4831844Z The following packages will be downloaded:
2021-11-23T12:23:01.4832287Z 
2021-11-23T12:23:01.4832844Z     package                    |            build
2021-11-23T12:23:01.4833699Z     ---------------------------|-----------------
2021-11-23T12:23:01.4834640Z     llvmdev-11.1.0             |       he1b5a44_3       374.3 MB  numba
2021-11-23T12:23:01.4835592Z     ------------------------------------------------------------
2021-11-23T12:23:01.4836332Z                                            Total:       374.3 MB
2021-11-23T12:23:01.4836829Z 
2021-11-23T12:23:01.4837394Z The following NEW packages will be INSTALLED:
2021-11-23T12:23:01.4837907Z 
2021-11-23T12:23:01.4838694Z   llvmdev            numba/linux-64::llvmdev-11.1.0-he1b5a44_3

whereas it's build 4 that contains the additional cross targets and they are all in numba/label/ci ?

esc · 2021-11-23T14:44:53Z

Is the issue that the build scripts are looking in channel numba and picking up build 3: https://github.com/numba/llvmlite/blob/master/buildscripts/incremental/setup_conda_environment.sh#L30 Log extract:

2021-11-23T12:23:01.4828781Z   added / updated specs:
2021-11-23T12:23:01.4830437Z     - llvmdev=11
2021-11-23T12:23:01.4830913Z 
2021-11-23T12:23:01.4831264Z 
2021-11-23T12:23:01.4831844Z The following packages will be downloaded:
2021-11-23T12:23:01.4832287Z 
2021-11-23T12:23:01.4832844Z     package                    |            build
2021-11-23T12:23:01.4833699Z     ---------------------------|-----------------
2021-11-23T12:23:01.4834640Z     llvmdev-11.1.0             |       he1b5a44_3       374.3 MB  numba
2021-11-23T12:23:01.4835592Z     ------------------------------------------------------------
2021-11-23T12:23:01.4836332Z                                            Total:       374.3 MB
2021-11-23T12:23:01.4836829Z 
2021-11-23T12:23:01.4837394Z The following NEW packages will be INSTALLED:
2021-11-23T12:23:01.4837907Z 
2021-11-23T12:23:01.4838694Z   llvmdev            numba/linux-64::llvmdev-11.1.0-he1b5a44_3

whereas it's build 4 that contains the additional cross targets and they are all in numba/label/ci ?

good catch, that may very well be the case!

esc · 2021-11-23T17:03:08Z

/azp run

azure-pipelines · 2021-11-23T17:03:18Z

Azure Pipelines successfully started running 1 pipeline(s).

gmarkall · 2021-11-23T17:38:12Z

All tests passed now that the llvmdev packages are available on the main channel - many thanks @esc!

esc · 2021-11-23T17:53:03Z

All tests passed now that the llvmdev packages are available on the main channel - many thanks @esc!

👌 Excellent!

esc · 2021-11-23T17:59:24Z

@occheung @sbourdeauducq @gmarkall thank you all!

targets: pass abiname param

1dc90f7

esc added the 3 - Ready for Review label Oct 12, 2021

occheung mentioned this pull request Nov 8, 2021

Add floating-point instruction support to kernel for non-Kasli-v1.x targets using VexRiscv core m-labs/artiq#1777

Merged

8 tasks

occheung force-pushed the riscv-abiname branch 2 times, most recently from ef8d72f to 864c95f Compare November 16, 2021 09:38

gmarkall added 4 - Waiting on reviewer and removed 3 - Ready for Review labels Nov 16, 2021

seibert mentioned this pull request Nov 16, 2021

Add RISC-V + all default targets to LLVM build #788

Merged

gmarkall requested changes Nov 18, 2021

View reviewed changes

occheung added 2 commits November 22, 2021 10:20

test/binding: test riscv asm

7ad8c8a

llvmdev: add RISCV target

a8959fe

doc: include abiname option

4a04450

occheung force-pushed the riscv-abiname branch from 42fbdcf to 4a04450 Compare November 22, 2021 02:26

gmarkall approved these changes Nov 22, 2021

View reviewed changes

gmarkall added 4 - Waiting on CI and removed 4 - Waiting on reviewer labels Nov 22, 2021

gmarkall added this to the Version 0.38.0 RC milestone Nov 23, 2021

Merge remote-tracking branch 'origin/master' into riscv-abiname

54d4967

gmarkall mentioned this pull request Nov 23, 2021

Test #775 on CI (Add abiname) #793

Closed

gmarkall added 5 - Ready to merge and removed 4 - Waiting on CI labels Nov 23, 2021

esc merged commit 91f7b6d into numba:master Nov 23, 2021

targets.py: Add ABIName parameter for RISC-V hard float targets #775

targets.py: Add ABIName parameter for RISC-V hard float targets #775

Conversation

occheung commented Oct 12, 2021

esc commented Oct 12, 2021

sbourdeauducq commented Nov 8, 2021 • edited Loading

esc commented Nov 8, 2021

gmarkall commented Nov 8, 2021

sbourdeauducq commented Nov 8, 2021

gmarkall commented Nov 15, 2021

sbourdeauducq commented Nov 15, 2021 • edited Loading

gmarkall commented Nov 15, 2021

sbourdeauducq commented Nov 15, 2021

gmarkall commented Nov 15, 2021

seibert commented Nov 15, 2021

occheung commented Nov 16, 2021

sbourdeauducq commented Nov 16, 2021

occheung commented Nov 16, 2021 • edited Loading

gmarkall commented Nov 16, 2021

gmarkall commented Nov 16, 2021

gmarkall commented Nov 16, 2021

esc commented Nov 16, 2021

sbourdeauducq commented Nov 16, 2021

gmarkall left a comment

Choose a reason for hiding this comment

gmarkall commented Nov 22, 2021

gmarkall left a comment

Choose a reason for hiding this comment

gmarkall commented Nov 23, 2021

azure-pipelines bot commented Nov 23, 2021

esc commented Nov 23, 2021

azure-pipelines bot commented Nov 23, 2021

esc commented Nov 23, 2021

esc commented Nov 23, 2021

gmarkall commented Nov 23, 2021

stuartarchibald commented Nov 23, 2021

esc commented Nov 23, 2021

esc commented Nov 23, 2021

azure-pipelines bot commented Nov 23, 2021

gmarkall commented Nov 23, 2021

esc commented Nov 23, 2021

esc commented Nov 23, 2021

sbourdeauducq commented Nov 8, 2021 •

edited

Loading

sbourdeauducq commented Nov 15, 2021 •

edited

Loading

occheung commented Nov 16, 2021 •

edited

Loading