Page MenuHomePhabricator

[RISCV-V] Provide muldi3 builtins for riscv
ClosedPublic

Authored by kamleshbhalui on May 22 2020, 7:54 PM.

Details

Summary

Compiling for riscv without M extension results in below problem.

00000000000100fc <__udivdi3>:
...

101b4:       05c2                    slli    a1,a1,0x10
101b6:       10158593                addi    a1,a1,257
101ba:       11e000ef                jal     ra,102d8 <__muldi3>

and

00000000000102d8 <__muldi3>:

102d8:       711d                    addi    sp,sp,-96
102da:       ec86                    sd      ra,88(sp)

...

10324:       8526                    mv      a0,s1
10326:       85da                    mv      a1,s6
10328:       fb1ff0ef                jal     ra,102d8 <__muldi3>
1032c:       9562                    add     a0,a0,s8
1032e:       01055c1b                srliw   s8,a0,0x10
10332:       00857d33                and     s10,a0,s0
10336:       010bd513                srli    a0,s7,0x10
1033a:       008574b3                and     s1,a0,s0
1033e:       8526                    mv      a0,s1
10340:       85d2                    mv      a1,s4
10342:       f97ff0ef                jal     ra,102d8 <__muldi3>

...
We can see __muldi3 call itself endlessly.
It will fix this https://bugs.llvm.org/show_bug.cgi?id=43388 too.

Hence this patch.

Diff Detail

Event Timeline

kamleshbhalui created this revision.May 22 2020, 7:54 PM

You can generalize muldi3.c to implement __mulsi3.

You can generalize muldi3.c to implement __mulsi3.

looks like no one uses mulsi3 other than riscv32 which already has target specific implementation.
This patch adds
muldi3 target-specific implementation for riscv64 to get rid of problem shown in summary.

added CMakeLists.txt change for riscv64.

kamleshbhalui edited the summary of this revision. (Show Details)May 23 2020, 2:21 AM

You can generalize muldi3.c to implement __mulsi3.

looks like no one uses mulsi3 other than riscv32 which already has target specific implementation.
This patch adds
muldi3 target-specific implementation for riscv64 to get rid of problem shown in summary.

Still uneasy with the assembly implementation. .c works as well, doesn't it?

You can generalize muldi3.c to implement __mulsi3.

looks like no one uses mulsi3 other than riscv32 which already has target specific implementation.
This patch adds
muldi3 target-specific implementation for riscv64 to get rid of problem shown in summary.

Still uneasy with the assembly implementation. .c works as well, doesn't it?

When compile muldi3.c with disable M Extension there is self call loop in __muldi3 routine .

luismarques requested changes to this revision.May 27 2020, 8:43 AM

I think it would make sense to merge this with the mulsi3 implementation, since it's the same sequence of instructions.
Like the mulsi3_test.c, you should provide a test for this. The mulsi3_test.c was provided as a RISC-V-specific test, but there doesn't seem to be anything RISC-V specific in the test, so it should probably just be a generic test (hat tip to Alex Bradbury for noticing that). Likewise, the same would go for your muldi3 test.

This revision now requires changes to proceed.May 27 2020, 8:43 AM
MaskRay added inline comments.May 27 2020, 10:04 PM
compiler-rt/test/builtins/Unit/riscv/muldi3_test.c
1 ↗(On Diff #266731)

I know Unit/arm set an example but the more common directory name in llvm-project is uppercase RISCV/

23 ↗(On Diff #266731)

2 space indentation even if some files don't obey the llvm-project rule.

When I said earlier that you should provide a muldi3 test I was saying that under the impression that there wasn't one already. It seems that there is already a target-indepent test. Sorry about that. My point was that the test did NOT have to be RISC-V specific. The generic test is gated on librt_has_muldi3. For that to work with lit automatically, your implementation files have to have the correct name. See compiler-rt/test/builtins/CMakeLists.txt, specifically the comment saying "hexagon/udivsi3.S" => "udivsi3". Others can comment on what is the best/cleanest way forward:

  1. Keep the patch as is, with a RISC-V-specific test.
  2. Have mulsi3.S and muldi3.S and duplicate the code. This should work with the generic muldi3 test.
  3. Have mulsi3.S and muldi3.S and one #includes the other's code. This should work with the generic muldi3 test.

I would lean towards 3. There may be a way to have a single .S file and still get the librt_has_muldi3, but I'm not seeing it right now.

The combined .S implementation seems fine, thanks for doing that!

restructured the file. so it can be picked by lit test.

luismarques accepted this revision.May 28 2020, 7:43 AM

LGTM. But please give it some time for other people to chime in before you merge, they may disagree with this .inc approach.
I'm approving this assuming the generic test is actually ran against this implementation. @kamleshbhalui did you verify that was the case?

This revision is now accepted and ready to land.May 28 2020, 7:43 AM
luismarques added inline comments.May 28 2020, 7:59 AM
compiler-rt/lib/builtins/riscv/muldi3.S
2

duplicate routines routines

compiler-rt/lib/builtins/riscv/mulsi3.S
1

routines, routines everywhere! :)

LGTM. But please give it some time for other people to chime in before you merge, they may disagree with this .inc approach.
I'm approving this assuming the generic test is actually ran against this implementation. @kamleshbhalui did you verify that was the case?

yes @luismarques I have tested the generic test against this implementation.

I do not have commit access,
can someone commit this for me?

I do not have commit access,
can someone commit this for me?

I can commit that for you.

Tip: in future patches be sure to provide the full context (use something like -U99999999 with git) so that the full source code of the changed files is available in Phabricator.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 1:10 PM
Herald added subscribers: Restricted Project, jrtc27. · View Herald Transcript

Thanks. I am also happy with the updated version.

I noticed that this patch (along with D86036) caused a compiler-rt test to fail for riscv64 when M extension is enabled.

compiler-rt/test/builtins/Unit/muldi3_test.c:11: undefined reference to `__muldi3'

Basically, with this patch, __muldi3 is no longer defined anymore for riscv64 when M extension is enabled. Any suggestion on how this should be fixed?

I noticed that this patch (along with D86036) caused a compiler-rt test to fail for riscv64 when M extension is enabled.
compiler-rt/test/builtins/Unit/muldi3_test.c:11: undefined reference to `muldi3'
Basically, with this patch,
muldi3 is no longer defined anymore for riscv64 when M extension is enabled. Any suggestion on how this should be fixed?

That is fixed by my patch in D86457. (It hasn't yet been approved after I addressed some initial review comments, but I think it's now in good shape to land).

I noticed that this patch (along with D86036) caused a compiler-rt test to fail for riscv64 when M extension is enabled.

compiler-rt/test/builtins/Unit/muldi3_test.c:11: undefined reference to `__muldi3'

Basically, with this patch, __muldi3 is no longer defined anymore for riscv64 when M extension is enabled. Any suggestion on how this should be fixed?

Do other architectures with native 64-bit multiplication provide __muldi3? If yes, it should be re-enabled; if no, the test should be fixed. My assumption is it's the latter, but I can see the argument for having an RVM compiler-rt still provide __muldi3 in case linked against code that was compiled without RVM (but, really, that code should just be compiled with RVM).

I noticed that this patch (along with D86036) caused a compiler-rt test to fail for riscv64 when M extension is enabled.
compiler-rt/test/builtins/Unit/muldi3_test.c:11: undefined reference to `muldi3'
Basically, with this patch,
muldi3 is no longer defined anymore for riscv64 when M extension is enabled. Any suggestion on how this should be fixed?

That is fixed by my patch in D86457. (It hasn't yet been approved after I addressed some initial review comments, but I think it's now in good shape to land).

Glad to hear that this is already being worked on, @luismarques!

luismarques added a comment.EditedSep 18 2020, 4:09 PM

Do other architectures with native 64-bit multiplication provide __muldi3? If yes, it should be re-enabled; if no, the test should be fixed. My assumption is it's the latter, but I can see the argument for having an RVM compiler-rt still provide __muldi3 in case linked against code that was compiled without RVM (but, really, that code should just be compiled with RVM).

We discussed that in one of the RISC-V sync up calls. People seemed to be in agreement that always providing the builtin was the way to go.