This is an archive of the discontinued LLVM Phabricator instance.

Differential D80465

[RISCV-V] Provide muldi3 builtins for riscv
ClosedPublic

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

Details

Reviewers
kito-cheng
mgrang
luismarques
Commits
rG5fa1f1e9f44b: [RISCV-V] Provide muldi3 builtin assembly implementation
rGe31ccee1b01a: [RISCV-V] Provide muldi3 builtin assembly implementation
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
Herald added subscribers: evandro, apazos, sameer.abuasal and 22 others. · View Herald TranscriptMay 22 2020, 7:54 PM
MaskRay added a subscriber: MaskRay.May 22 2020, 9:10 PM

You can generalize muldi3.c to implement __mulsi3.

kamleshbhalui added a comment.May 23 2020, 2:16 AM
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 updated this revision to Diff 265843.May 23 2020, 2:19 AM

added CMakeLists.txt change for riscv64.

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

ping?

MaskRay added a comment.May 27 2020, 8:02 AM
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 added a comment.May 27 2020, 8:20 AM
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 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
kamleshbhalui updated this revision to Diff 266731.May 27 2020, 9:33 PM

Addressed @luismarques concerns.

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.

luismarques added a comment.May 27 2020, 11:14 PM

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 updated this revision to Diff 266872.May 28 2020, 7:28 AM

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
1

duplicate routines routines

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

routines, routines everywhere! :)

kamleshbhalui updated this revision to Diff 267456.May 30 2020, 6:46 AM

cleanup

kamleshbhalui added a comment.Jun 2 2020, 9:58 AM
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 added a comment.Jun 2 2020, 10:00 AM

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

luismarques added a comment.Jun 2 2020, 12:42 PM
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.

Closed by commit rGe31ccee1b01a: [RISCV-V] Provide muldi3 builtin assembly implementation (authored by kamleshbhalui, committed by luismarques). · Explain WhyJun 2 2020, 1:10 PM
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
MaskRay added a comment.Jun 2 2020, 1:27 PM

Thanks. I am also happy with the updated version.

luismarques mentioned this in D86036: [compiler-rt][RISCV] Use muldi3 builtin assembly implementation.Aug 16 2020, 6:16 AM
luismarques mentioned this in rG57903cf09335: [compiler-rt][RISCV] Use muldi3 builtin assembly implementation.Aug 21 2020, 5:07 AM
pzheng added a comment.Sep 18 2020, 3:58 PM

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 added a comment.Sep 18 2020, 4:03 PM
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 added a comment.Sep 18 2020, 4:03 PM
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 added a comment.Sep 18 2020, 4:06 PM
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 added a comment.EditedSep 18 2020, 4:09 PM
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.

Revision Contents

PathSize
compiler-rt/
lib/
builtins/
riscv/
31 lines
11 lines
21 lines

Diff 266872

compiler-rt/lib/builtins/riscv/int_mul_impl.inc

  • This file was added.
//===-- int_mul_impl.inc - Integer multiplication -------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//
//
// Helpers used by __mulsi3, __muldi3.
//
//===----------------------------------------------------------------------===//
#if !defined(__riscv_mul)
.text
.align 2
.globl __mulxi3
.type __mulxi3, @function
__mulxi3:
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

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

  • This file was added.
//===--- muldi3.S - Integer multiplication routines routines ---===//
luismarquesUnsubmitted
Not Done
ReplyInline Actions

duplicate routines routines

luismarques: duplicate 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 __riscv_xlen == 64
#define __mulxi3 __muldi3
#include "int_mul_impl.inc"
#endif

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

//===--- mulsi3.S - Integer multiplication routines routines ---===// //===--- mulsi3.S - Integer multiplication routines routines ---===//
luismarquesUnsubmitted
Not Done
ReplyInline Actions

routines, routines everywhere! :)

luismarques: routines, routines everywhere! :)
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#if !defined(__riscv_mul) && __riscv_xlen == 32 #if __riscv_xlen == 32
.text #define __mulxi3 __mulsi3
.align 2 #include "int_mul_impl.inc"
.globl __mulsi3
.type __mulsi3, @function
__mulsi3:
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 #endif