[RISCV-V] Provide muldi3 builtins for riscv

[kamleshbhalui]

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.

[MaskRay]

You can generalize muldi3.c to implement __mulsi3.

[kamleshbhalui]

In D80465#2051925, @MaskRay wrote:

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.

[kamleshbhalui]

added CMakeLists.txt change for riscv64.

[kamleshbhalui]

ping?

[MaskRay]

In D80465#2051978, @kamleshbhalui wrote:

In D80465#2051925, @MaskRay wrote:

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?

[kamleshbhalui]

In D80465#2057408, @MaskRay wrote:

In D80465#2051978, @kamleshbhalui wrote:

In D80465#2051925, @MaskRay wrote:

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]

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.

[kamleshbhalui]

Addressed @luismarques concerns.

[MaskRay]

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.

[luismarques]

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!

[kamleshbhalui]

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

[luismarques]

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?

[luismarques]

compiler-rt/lib/builtins/riscv/muldi3.S

2

duplicate routines routines

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

1 ↗

(On Diff #266872)

routines, routines everywhere! :)

[kamleshbhalui]

cleanup

[kamleshbhalui]

In D80465#2060164, @luismarques wrote:

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.

[kamleshbhalui]

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

[luismarques]

In D80465#2069080, @kamleshbhalui wrote:

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.

[MaskRay]

Thanks. I am also happy with the updated version.

[pzheng]

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?

[luismarques]

In D80465#2283180, @pzheng wrote:

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).

[jrtc27]

In D80465#2283180, @pzheng wrote:

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).

[pzheng]

In D80465#2283183, @luismarques wrote:

In D80465#2283180, @pzheng wrote:

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]

In D80465#2283185, @jrtc27 wrote:

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.

compiler-rt/lib/builtins/CMakeLists.txt

   riscv/mulsi3.S
   ${riscv_SOURCES}
 )
-set(riscv64_SOURCES ${riscv_SOURCES})
+set(riscv64_SOURCES
+  riscv/muldi3.S
+  ${riscv_SOURCES}
+)
 
 set(sparc_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})
 set(sparcv9_SOURCES ${GENERIC_SOURCES} ${GENERIC_TF_SOURCES})

compiler-rt/lib/builtins/riscv/muldi3.S

+//===--- muldi3.S - Integer multiplication routines routines ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#if !defined(__riscv_mul) && __riscv_xlen == 64
+  .text
+  .align 2
+
+  .globl __muldi3
+  .type  __muldi3, @function
+__muldi3:
+  mv     a2, a0
+  mv     a0, zero
+.L1:
+  andi   a3, a1, 1
+  beqz   a3, .L2
+  add    a0, a0, a2
+.L2:
+  srli   a1, a1, 1
+  slli   a2, a2, 1
+  bnez   a1, .L1
+  ret
+#endif