Page MenuHomePhabricator

[Sparc] Add Soft Float support
ClosedPublic

Authored by jacob_hansen on Apr 19 2016, 8:33 AM.

Details

Summary

This patch adds support for software floating point operations for Sparc targets.

This is the first in a set of patches to enable software floating point on Sparc. The next patch will enable the option to be used with Clang.

Diff Detail

Event Timeline

jacob_hansen retitled this revision from to [Sparc] Add Soft Float support.
jacob_hansen updated this object.
jacob_hansen added a reviewer: jyknight.
jacob_hansen added a subscriber: llvm-commits.

Reintroduced the header on line 1 of SparcISelLowering.h which was accidentally removed.

jacob_hansen updated this object.

I had initially planned to do this over two patches but changed my mind a decided it was better to do it in one. This updated patch ensures that the soft-float attribute is retained when the target options are being reset (this change is identical to how other targets handles this situation).

lero_chris accepted this revision.Apr 21 2016, 8:28 AM
lero_chris added a reviewer: lero_chris.
This revision is now accepted and ready to land.Apr 21 2016, 8:28 AM
jacob_hansen edited edge metadata.
jyknight edited edge metadata.Apr 25 2016, 8:59 PM
jyknight added a subscriber: echristo.

What's the ABI impact of this? Will this use the same calling convention as GCC does?

lib/Target/Sparc/SparcTargetMachine.cpp
88

I wonder what the point of having this redundant way of specifying soft-float via "use-soft-float" is. Clang seems to emit *both* use-soft-float=true and +soft-float. Hopefully this isn't needed these days. @echristo?

lib/Target/Sparc/SparcTargetMachine.h
25–26

Pretty sure this ought to be deleted while adding the SubtargetMap.

The change should use the same calling convention as GCC does, yes. I tested the change by linking with libgcc (from http://www.gaisler.com/index.php/products/operating-systems/bcc) which contains implementation of all the floating point procedures, that seemed to work perfectly. I also tested it by manually compiling a library of the builtins from the Compiler-RT project (Compiler-RT builtins are compatible with libgcc as noted on http://compiler-rt.llvm.org/), and that worked as well.

lib/Target/Sparc/SparcTargetMachine.cpp
88

I was wondering as well actually, didn't really find an answer to why this might be, but all targets seem to do this at the moment.

lib/Target/Sparc/SparcTargetMachine.h
25–26

I think you are right. Currently I use that object on line 100 of SparcTargetMachine.cpp to determine whether the target is currently 32 or 64 bit (using is64Bit()). Should I just create a new boolean variable here to determine this value instead of using the Subtarget object, or is there a better way?

echristo added inline comments.May 1 2016, 10:13 PM
lib/Target/Sparc/SparcSubtarget.cpp
56

Can you do this constification in a separate patch? (Feel free to just submit it).

lib/Target/Sparc/SparcTargetMachine.cpp
88

It's redundant as a subtarget feature for soft float wasn't across all targets yet, but if someone is willing to do that cleanup we can get rid of it. (Should do this in a separate patch as I know there are out of tree users etc...)

lib/Target/Sparc/SparcTargetMachine.h
25–26

This should be removed for sure. You should determine 32 or 64-bit ness in the TargetMachine based on what information you have in the TargetMachine configuration rather than the other. It can't change on a subtarget specific basis anyhow.

jacob_hansen added inline comments.
lib/Target/Sparc/SparcSubtarget.cpp
56

Sure, I submitted D19797 - This was my first patch so I do not have SVN write access at this time, could you submit it for me?

jacob_hansen edited edge metadata.

Removed the subtarget object from the TargetMachine and changed the way 32/64 bit ness is determined as requested.

The constification of TargetMachine arguments in SparcISelLowering and SparcSubtarget has been moved to D19797, which is now a dependency for this patch.

Looks pretty good to me, but here's a few issues:

  1. There's a *bunch* of trailing whitespace in your patch (e.g. most of getSubtargetImpl). If you run your patch through clang-format, it will fix that for you, either use git clang-format (if you're using git for your checkout -- which I'd recommend), or clang-format-diff as mentioned on http://clang.llvm.org/docs/ClangFormat.html, if you're using svn.
  1. Your CHECKs in your test are incorrectly specified -- CHECK-LABEL is to specify the beginning of the function, *not* for the assertion itself. See http://llvm.org/docs/CommandGuide/FileCheck.html
  1. You can simplify your test cases by getting rid of the alloca and loads: just move the values to function arguments.

Like this, for example:

 define i1 @test_gtdf2(double %a, double %b) #0 {
; CHECK-LABEL: test_gtdf2:
; CHECK: call __gtdf2
  %cmp = fcmp ogt double %a, %b
  ret i1 %cmp
}
  1. There are no function call/return ABI tests. Can probably add to the existing 32abi.ll and 64abi.ll tests for this, by adding another RUN line.

E.g. for 64abi.ll

; RUN: llc < %s -march=sparcv9 -disable-sparc-delay-filler -disable-sparc-leaf-proc | FileCheck %s --check-prefix=CHECK --check-prefix=HARD
; RUN: llc < %s -march=sparcv9 -disable-sparc-delay-filler -disable-sparc-leaf-proc  -mattr=soft-float | FileCheck %s --check-prefix=CHECK --check-prefix=SOFT

And then you just expand the CHECK: lines which differ for hard/soft-float to have a separate HARD: and SOFT: case, instead.

  1. No tests for float or fp128. For fp128, it might make sense to have another RUN line on the existing tests in fp128.ll.
  1. Nit: I'd rename the file "soft-float.ll", vs "softfloat.ll"
jacob_hansen marked 5 inline comments as done.

The following have been updated in this patch:

  • trailing whitespace removed
  • soft-float.ll tests is now correctly specified, the test cases have been simplified, added tests for fp128, and file was renamed from softfloat.ll
  • adjusted the test cases in 32abi.ll and 62abi.ll to include testing for software floating point
  • Adjusted behavior for fp128/quad floats so that the compiler-rt function calls (__addtf3 and so forth) are used for all floating point operations, including fp128, when soft-float option is enabled (before calls to _Q_add was used for fp128/quad floats when softfloat was enabled)

Accidentally removed two tests from 32abi.ll in the previous revision, this revision reintroduces them.

lero_chris closed this revision.May 18 2016, 12:51 AM