Page MenuHomePhabricator

[compiler-rt][builtins][RISCV] Always include __mul[sd]i3 builtin definitions
ClosedPublic

Authored by luismarques on Aug 24 2020, 6:28 AM.

Details

Summary

The RISC-V implementations of the __mulsi3, __muldi3 builtins were conditionally compiling the actual function definitions depending on whether the M extension was present or not. This caused Compiler-RT testing failures for RISC-V targets with the M extension, as when these sources were included the librt_has_mul*i3 features were still being defined. These librt_has_* definitions are used to conditionally run the respective tests. Since the actual functions were not being compiled-in, the generic test for __muldi3 would fail. This patch makes these implementations follow the normal Compiler-RT convention of always including the definition, and conditionally running the respective tests by using the lit conditional REQUIRES: librt_has_*.

Since the mulsi3_test.c wasn't actually RISC-V-specific, this patch also moves it out of the riscv directory. It now only depends on librt_has_mulsi3 to run.

Diff Detail

Event Timeline

luismarques created this revision.Aug 24 2020, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 6:28 AM
Herald added subscribers: Restricted Project, evandro, apazos and 22 others. · View Herald Transcript
luismarques requested review of this revision.Aug 24 2020, 6:28 AM

Two nits about the implementations. Otherwise, LGTM!

compiler-rt/lib/builtins/riscv/int_mul_impl.inc
12–16

I wonder if this file should have a stanza at the top something like the following (which should make it clearer how this file is used):

#ifndef __mulxi3
#error "__mulxi3 must be defined to use this generic implementation"
#endif
compiler-rt/test/builtins/Unit/mulsi3_test.c
9

I went looking for the file this comment references and could not find it. The only file with this filename is the current file?!

Anyway this is all to say: this comment was confusing and thanks for deleting it.

18

I *think* you should be using x here rather than re-calling __mulsi3.

luismarques added inline comments.Sep 8 2020, 6:39 AM
compiler-rt/test/builtins/Unit/mulsi3_test.c
9

Indeed. Before I submitted this patch I checked in the git history if such a file existed when this test was originally added, and it did not. I think it was supposed to say it was based on muldi3_test.c instead. It's probably best to just remove it. I imagine that people who care about this file will also know to look at muldi3_test.c, especially now that this patch moves this test to a target-independent location.

luismarques retitled this revision from [compiler-rt][builtins][RISCV] Always include __mul[sd]i3 builtin definition to [compiler-rt][builtins][RISCV] Always include __mul[sd]i3 builtin definitions.

Address review concerns.

luismarques marked 3 inline comments as done.Sep 8 2020, 6:41 AM
asb accepted this revision.Oct 15 2020, 4:49 AM

This LGTM. @lenary are you happy your comments are addressed?

This revision is now accepted and ready to land.Oct 15 2020, 4:49 AM
lenary accepted this revision.Oct 15 2020, 5:14 AM

LGTM. Yes you got all my changes.

This revision was landed with ongoing or failed builds.Oct 21 2020, 1:49 AM
This revision was automatically updated to reflect the committed changes.