[PATCH] D19265: [Sparc] Add Soft Float support

[James Y Knight via llvm-commits]

jyknight added a comment.

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.

2. 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

3. 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
  }

5. 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.

5. No tests for float or fp128. For fp128, it might make sense to have another RUN line on the existing tests in fp128.ll.

6. Nit: I'd rename the file "soft-float.ll", vs "softfloat.ll"


http://reviews.llvm.org/D19265