Page MenuHomePhabricator

[Analysis] enhance FP library function prototype checking to match types with name suffix
AbandonedPublic

Authored by spatel on Jun 13 2019, 12:13 PM.

Details

Summary

I missed some existing test diffs in D63214 (not currently shown there), and this is the root cause: we loosely matched libm function names without checking that the return/parameter types correspond to the name.

The rule is:

  1. No suffix --> double
  2. 'f' suffix --> float
  3. 'l' suffix --> long double

I'm not sure how to deal with long double yet, so I left that as TODOs.

The AMDGPU tests are adjusted to have the correct libm names without changing anything else. The instcombine sqrt test is the intentional diff. There are more comprehensive existing instcombine tests that will fail when transforming double functions to float functions, so I didn't bother adding tests for each name.

Diff Detail

Event Timeline

spatel created this revision.Jun 13 2019, 12:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 12:13 PM
cameron.mcinally accepted this revision.Jun 18 2019, 9:09 AM

LGTM, assuming the test writer was not intentionally playing games with the fabs(...) calls.

This revision is now accepted and ready to land.Jun 18 2019, 9:09 AM

AVR intentionally makes the C type "double" a single-precision float ("float" in IR). So we could fail to recognize "fabs" etc. on AVR with this patch, I think?

AVR intentionally makes the C type "double" a single-precision float ("float" in IR). So we could fail to recognize "fabs" etc. on AVR with this patch, I think?

How do we test that? If that's a problem here, we might have an existing problem in clang because we convert to LLVM intrinsics in clang/lib/CodeGen using similar checks.

Note that this an extension of the existing checks, and we have seen bugs caused by matching libcall signatures too loosely:
rL258428
D16198 / rL258325

How do we test that?

The following should produce ret float 0.000000e+00:

echo "double sqrt(double x); double z() { return sqrt(0); }" | clang --target=avr -x c - -o - -S -emit-llvm -O2

Not sure what you're talking about with clang; it does the right thing, as far as I can tell.

Not sure what you're talking about with clang; it does the right thing, as far as I can tell.

When compiling for AVR, clang will turn the libcall that uses doubles in C source into:

%call = call addrspace(1) float @sqrt(float 0.000000e+00) #2

Should clang translate that call into "sqrtf" to be more accurate? Similarly, if the source was written with "sqrtl" and "long double", we'd get this IR out of clang:

%call = call addrspace(1) float @sqrtl(float 0.000000e+00) #2

Not sure what you're talking about with clang; it does the right thing, as far as I can tell.

When compiling for AVR, clang will turn the libcall that uses doubles in C source into:

%call = call addrspace(1) float @sqrt(float 0.000000e+00) #2

Should clang translate that call into "sqrtf" to be more accurate? Similarly, if the source was written with "sqrtl" and "long double", we'd get this IR out of clang:

%call = call addrspace(1) float @sqrtl(float 0.000000e+00) #2

And I should've mentioned this in the earlier comment, both of those calls can be translated to the LLVM sqrt intrinsic (if we have no-errno) because checking for builtins appears to be done solely by name:

bool Builtin::Context::isBuiltinFunc(const char *Name) {
  StringRef FuncName(Name);
  for (unsigned i = Builtin::NotBuiltin + 1; i != Builtin::FirstTSBuiltin; ++i)
    if (FuncName.equals(BuiltinInfo[i].Name))
      return strchr(BuiltinInfo[i].Attributes, 'f') != nullptr;

  return false;
}

Should clang translate that call into "sqrtf" to be more accurate?

On a target where sqrtf/sqrt/sqrtl have argument and return types with the same representation, we could rewrite the name of the function declaration, but I don't see why we would. The C type "float" and the LLVM IR type "float" are not the same thing in general; the suffix denotes the C type, not the LLVM IR type.

spatel abandoned this revision.Jun 19 2019, 3:42 PM

Abandoning - please see D63214 for the revised test in test/Transforms/InstCombine/double-float-shrink-1.ll that this patch was seeking to preserve.