Page MenuHomePhabricator

[RISCV-V] Provide muldi3 builtins for riscv
AcceptedPublic

Authored by kamleshbhalui on Fri, May 22, 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.Fri, May 22, 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)Sat, May 23, 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.Wed, May 27, 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.Wed, May 27, 8:43 AM
MaskRay added inline comments.Wed, May 27, 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.Thu, May 28, 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.Thu, May 28, 7:43 AM
luismarques added inline comments.Thu, May 28, 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! :)