Page MenuHomePhabricator

[compiler-rt] Replaced __SOFT_FP__ with __SOFTFP__
ClosedPublic

Authored by kamleshbhalui on Jun 17 2020, 7:41 AM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=46294
As described in above bug.

in compiler-rt at few places we

__SOFT_FP__

macro used, but never defined any where
either by compiler or config files.

But I do find a similar macro

__SOFTFP__

(missing middle underscore) which defined by compiler.
so in this patch I replaced former macro with latter.

Diff Detail

Event Timeline

kamleshbhalui created this revision.Jun 17 2020, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 7:42 AM
Herald added subscribers: Restricted Project, dberris. · View Herald Transcript
kamleshbhalui edited the summary of this revision. (Show Details)Jun 17 2020, 7:42 AM
kamleshbhalui edited the summary of this revision. (Show Details)Jun 17 2020, 7:45 AM
kamleshbhalui removed a project: Restricted Project.
kamleshbhalui edited subscribers, added: llvm.org; removed: Restricted Project, Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 7:45 AM
kamleshbhalui edited the summary of this revision. (Show Details)Jun 17 2020, 7:46 AM
MaskRay added a comment.EditedJun 18 2020, 9:58 AM

Please upload with full diff diff -U9999, or use arc diff --head=HEAD 'HEAD~1'

If the original code intends to target __SOFTFP__ (compiler defined macro), then these are indeed typo errors. It seems that __SOFT_FP__ is used to pick different implementations. It is likely that the #ifdef __SOFT_FP__ branches are never executed, so the potential correctness/accuracy issues may never be exposed. Even if __SOFT_FP__ uses are indeed typo errors (I am unsure as I can't find __SOFTFP__ in libgcc sources), we should test these on ARM softfp machines to ensure nothing breaks.

MaskRay edited reviewers, added: efriedma, psmith; removed: peter.smith.Jun 18 2020, 9:58 AM

I think that this is highly likely to be a typo, it looks like it dates to the contribution of compiler-rt so there isn't any history we can use. I believe that compiler-rt builtins originated in Apple so there is a possibility that SOFTFP was used in the compiler at contribution time. If you wanted to be safe you could support both SOFTFP and SOFT_FP but I don't think that is necessary. I would like to think that for a soft-fp target the tests for these functions fail or are at least xfailed at the moment. Are you able to compile the tests for a soft-fp target to check?

Ping @kamleshbhalui

I hope your or some reviewer can test the buildability on ARM.

Ping @kamleshbhalui

I hope your or some reviewer can test the buildability on ARM.

If I get some time this week(end) I'll make a soft-float build. I don't want to hold the change up by insisting we test on a soft-float Arm. I am interested in whether these functions are tested at all in that case, which we ought to follow up with if they are not.

Ping @kamleshbhalui

I hope your or some reviewer can test the buildability on ARM.

If I get some time this week(end) I'll make a soft-float build. I don't want to hold the change up by insisting we test on a soft-float Arm. I am interested in whether these functions are tested at all in that case, which we ought to follow up with if they are not.

In case it may be relevant, I faced something like this while trying to port the builtins library of compiler-rt to MSP430. At some point, I was debugging failing unit tests and spotted some functions not expecting the soft-float by default. That time I just conditionally #defined the __SOFT_FP__ macro in int_lib.h for MSP430 target and eventually made all the XPASS tests pass.

PS: msp430-elf-gcc provided by TI seems not to define such a macro:

echo | msp430-elf-gcc -E -dM - | grep SOFT
<prints nothing>
MaskRay accepted this revision.Sep 20 2020, 10:33 AM
This revision is now accepted and ready to land.Sep 20 2020, 10:33 AM
This revision was landed with ongoing or failed builds.Mon, Feb 22, 10:57 PM
This revision was automatically updated to reflect the committed changes.