This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][builtins][MIPS] Add mips16 wrappers for float arithmetic, comparison and conversion
Needs ReviewPublic

Authored by farazs on Jul 10 2014, 2:51 AM.

Details

Reviewers
howard.hinnant
Summary

This patch adds mips16 wrappers for floating-point arithmetic, comparison and type-conversion operations.

Background: mips16 code is not aware of floating point registers and call-conventions. For platforms equipped with hardware floating point, mips16 code resorts to calling an appropriate libgcc function with the float arguments in general purpose registers. The low-level function copies the arguments to FP registers, performs the operation and returns the result to the caller in a GP register.

Operations provided in this patch are:
Arithmetic(single & double precision): add, sub, mul, div
Comparison(single & double precision): eq, ne, gt, ge, lt, le, unord
Type conversions:

  • To signed integer: fix_truncXfsi
  • To float: floatunsiXf, floatsiXf
  • Within float formats: truncdfsf, extendsfdf

Patch for unit tests is: http://reviews.llvm.org/D4453

Diff Detail

Event Timeline

farazs updated this revision to Diff 11253.Jul 10 2014, 2:51 AM
farazs retitled this revision from to [compiler-rt][builtins][MIPS] Add mips16 wrappers for float arithmetic, comparison and conversion.
farazs updated this object.
farazs edited the test plan for this revision. (Show Details)
farazs added a reviewer: howard.hinnant.
farazs set the repository for this revision to rL LLVM.
farazs added subscribers: Unknown Object (MLST), dsanders.

CMake changes look fine. I don't really know anything about MIPS, so it would be great to get a quick look from someone knowledgeable. Daniel?

dsanders added inline comments.Jul 11 2014, 2:52 AM
lib/builtins/mips/mips16_cmpdf2.c
27–33

Shouldn't gtdf2 and gedf2 be the same function and return -2, -1, 0, or 1 for unordered, less-than, equal, or greater-than respectively (like glibc's soft float does)?

Similarly for ltdf2/ledf2 and the single precision equivalents.

lib/builtins/mips/mips16_gen.h
13–15

I understand what you mean but I think this ought to explain that the soft-fp wrappers make hardware floating point available to MIPS16 despite MIPS16 not supporting hardware floating point by temporarily switching the instruction set.

The header comments in the other files should mention this too.

23–24

This doesn't look right to me. Why not test using '#if !defined(mips16) || defined(mips_soft_float)'?

Also, I think it's easier to understand if the content of mips16_*.c is wrapped in an '#if defined(__mips16)' rather than defining a macro to expand to nothing.

Mostly agreed, further inputs on first point?

lib/builtins/mips/mips16_cmpdf2.c
27–33

I've used only this document as my reference.
Excerpt: `Do not rely on this implementation; only the semantics documented below are guaranteed. `

It seems like glibc's strategy is what is best-suited for a pure soft-float, whereas this implementation relies directly on the h/w comparison operations. As an example, this version doesn't need to check ordered-ness for every comparison operation because the underlying h/w operation takes care of it.

Do you foresee any disadvantages with this approach? I could compare space & time for both implementations, just to make sure we're not losing out.

lib/builtins/mips/mips16_gen.h
13–15

How about this (appropriately modified for all source files):

This file contains generator macros for soft-fp wrappers to make hardware 
floating point available to MIPS16 code by temporarily switching the instruction set.
These wrappers accept arguments and produce results in GP registers, but may 
perform the underlying operation using floating point instructions and registers.
23–24

Agreed on both counts.

dsanders added inline comments.Jul 14 2014, 2:18 AM
lib/builtins/mips/mips16_cmpdf2.c
27–33

Ah ok. I was looking at the comments in the implementation. Your current code looks correct to me.

Do you foresee any disadvantages with this approach?

Given that it's not needed for correctness, I believe matching glibc's soft-float would reduce performance with little or no improvement to code-size.

lib/builtins/mips/mips16_gen.h
13–15

Yes, that's ideal

farazs planned changes to this revision.Jul 15 2014, 1:27 AM
farazs added inline comments.
lib/builtins/mips/mips16_gen.h
23–24

After deeper inspection, using defined(mips16) is not suitable, because the library does not always need to be built in mips16 mode. The check is based on:

  1. if an application built with -mips16 can link with _this_ library. For example, if the library uses micromips or an ABI other than o32/o64, then a mips16 application will fail to link with it. I need to add a check for ABI.
  2. if the function needs to be defined. For example, all soft-fp wrappers are by definition redundant in soft-float mode as the compiler will never generate calls to those wrappers. Similarly, double-precision wrappers are redundant when building with -msingle-float -- this check needs to be added as well.

I could get rid of the empty-macros by conditionally #defining MIPS16_COMPAT in mips16_gen.h and wrapping contents of mips16_*.c in #ifdef MIPS16_COMPAT. Will include this and other changes in the next revision.

dsanders added inline comments.Jul 15 2014, 11:49 AM
lib/builtins/mips/mips16_gen.h
23–24

After deeper inspection, using defined(mips16) is not suitable, because the library does not always need to be built in mips16 mode.

Do you mean the library has to provide these functions regardless of the options used to compile the library just in case it is linked against a MIPS16 object/function that references them?

if an application built with -mips16 can link with _this_ library. For example, if the library uses micromips or an
ABI other than o32/o64, then a mips16 application will fail to link with it.

microMIPS and MIPS16 cannot coexist so you don't need to deal with that case. From the microMIPS spec:

microMIPS is the preferred replacement for MIPS16e. Therefore these two schemes never co-exist within the
same processor core

and later in the same document they both use the same ISA mode value:

The MIPS Release 3 architecture defines an ISA mode for each processor. An ISA mode value of 0 indicates MIPS32
instruction decoding. In processors implementing microMIPS32, an ISA mode value of 1 selects microMIPS32
instruction decoding. In processors implementing the MIPS16e ASE, an ISA mode value of 1 selects the decoding of
instructions as MIPS16e

Regarding the ABI comment: You can't check for mixing objects with different ABI's at compile-time. That needs to be a link-time check so you don't need to deal with that case in your code.

if the function needs to be defined. For example, all soft-fp wrappers are by definition redundant
in soft-float mode as the compiler will never generate calls to those wrappers. Similarly,
double-precision wrappers are redundant when building with -msingle-float -- this check needs
to be added as well.

You generally don't need a check to cover the cases where they are redundant. If they exist but the objects they are in are not referenced in some way then they won't be included in a link.

farazs added inline comments.Jul 15 2014, 11:34 PM
lib/builtins/mips/mips16_gen.h
23–24

After deeper inspection, using defined(mips16) is not suitable, because the library does not always need to be built in mips16 mode.

Do you mean the library has to provide these functions regardless of the options used to compile the library just in case it is linked against a MIPS16 object/function that references them?

Yes. These functions should be present by default, excluding specific cases where we know they are not needed or will not work. Excluded cases would be more obvious if the implementation were in assembly (as it is for libgcc) because they would either fail to build or behave incorrectly.

I think there is no harm in these functions being always available with every thing resolved correctly at link time. However, if exclusion checks are cheap and easy, we prefer to use them wherever possible simply to cut down on time needed for building/testing those cases. Even seemingly trivial savings in this regard can be significant on target boards.

dsanders added inline comments.Jul 21 2014, 9:05 AM
lib/builtins/mips/mips16_gen.h
23–24

Sorry for going quiet all of a sudden. Things got rather busy.

That makes sense to me. Excluding these functions when building the library for microMIPS or MIPS32r6/MIPS64r6 seems sensible too. I'm not sure about the soft-float check though. Admittedly it would be unusual, but I don't think there's a technical reason that an MIPS16 app couldn't be compiled with -mhard-float and link against a -msoft-float library. I believe it should end up with the app calling the hardfloat wrappers which are implemented using softfloat.

farazs updated this revision to Diff 14032.Sep 24 2014, 5:11 AM

Integrate suggestions from dsanders regarding comments and conditional compile blocks for mips16 wrappers. Introduce _MIPS16_COMPAT as a control-point for whether mips16 wrappers are needed in target library based on ABI and ISA revision.

sdkie added a subscriber: sdkie.Nov 4 2014, 3:16 AM