This is an archive of the discontinued LLVM Phabricator instance.

[TLI] Add prototype checking for all remaining LibFuncs.
ClosedPublic

Authored by ab on Dec 21 2016, 2:03 PM.

Details

Summary

This is another step towards unifying all LibFunc prototype checks (also see r267758): add the remaining checks to have some basic coverage for all LibFuncs.

This should be mostly straightforward, but I'm not sure the unittest is (can be made?) very valuable.
I'm hoping checking obviously broken prototypes would be the first step. Later, when fixes are inevitably made to tighten the TLI checks, this would be where tests go.

Thoughts?

Diff Detail

Event Timeline

ab updated this revision to Diff 82266.Dec 21 2016, 2:03 PM
ab retitled this revision from to [TLI] Add prototype checking for all remaining LibFuncs..
ab updated this object.
ab added reviewers: mehdi_amini, davide, efriedma.
ab added a subscriber: llvm-commits.
efriedma edited edge metadata.Dec 21 2016, 2:31 PM

Please put a bit more care into making sure signature checks you're adding are actually correct and reasonably precise.

lib/Analysis/TargetLibraryInfo.cpp
748

You could probably check these a bit more precisely.

1016

atan2 is wrong...

1080

sincospi_stret is wrong... I don't know the right signature off the top of my head, though.

davide edited edge metadata.Dec 22 2016, 9:27 AM

It would be great if you can add per-libcall tests to check they're correct (and make sure they won't bitrot).

lib/Analysis/TargetLibraryInfo.cpp
701–703

Maybe you can check also that the second argument to realloc() and reallocf() is an integer (size_t) ?
Also, isPointerTy() only matches void * or also T * with, let's say T = int or T = float?

1014–1016

atan2/atan2f/atan2l , they all take two arguments.

1124–1130

Are you sure isIntegerTy(32) is correct? In other words, is always correct to assume that an int is 32-bit ?

1175

Here the return type is int but you're just checking isIntegerTy().
In other places where the return type is int you check isIntegerTy(32).
Not sure which one is better, but we should be consistent.

davide requested changes to this revision.Dec 22 2016, 9:29 AM
davide edited edge metadata.
This revision now requires changes to proceed.Dec 22 2016, 9:29 AM
ab updated this revision to Diff 84198.Jan 12 2017, 5:06 PM
ab edited edge metadata.
  • Add positive tests for all libfuncs (mostly generated from OS X headers; some written by hand based on manpages)
  • Fix a couple incorrect checks, some mentioned in Eli's and Davide's reviews, some unearthed by the added tests
ab marked 4 inline comments as done.Jan 12 2017, 5:15 PM
ab added inline comments.
lib/Analysis/TargetLibraryInfo.cpp
748

That's true of a good portion of these libfuncs; I'm trying to fix it gradually! For that matter, some probably aren't even used anywhere in LLVM and should be removed.

1124–1130

I'm not sure, for 'int' or for any other C type ('char' is probably the only type for which LLVM already pervasively assumes a specific width).

I wouldn't be opposed to relaxing the checks; do you have a platform in mind?

This looks much better now.

lib/Analysis/TargetLibraryInfo.cpp
777–782

Please add a comment with the demangled name (as it's done in other places).

807–810

This is missing the check for the return type, int.

818

Can you narrow this? char * instead of isPointerTy()

912–915

please add a comment with the demangled name for our sanity =)

ab updated this revision to Diff 84344.Jan 13 2017, 11:42 AM
ab edited edge metadata.
  • Tighten checks for a couple functions
  • Replace isa<DerivedType> checks with ->isDerivedTy()
  • Generalize 'int' checks to isIntegerTy()
  • Add demangled name comments for C++ operators
ab marked 6 inline comments as done.Jan 13 2017, 11:45 AM
ab added inline comments.
lib/Analysis/TargetLibraryInfo.cpp
1124–1130

On second thought, I generalized these to isIntegerTy(). While I think it's a reasonable assumption to make for clang, it's much less so for TLI. More importantly, I skimmed a couple of my old TLI consolidation commits and didn't find i32 requirements.

ab updated this revision to Diff 84353.Jan 13 2017, 12:26 PM
ab marked an inline comment as done.

I spoke too soon! Tests finished running, and it turns out that a couple transforms do assume that int is i32. Add back the removed checks.

davide accepted this revision.Jan 14 2017, 12:58 PM
davide edited edge metadata.

I think this approached a state where it can be committed. I starred at this patch for too long though, (so I may be biased), wait until Eli takes another look.

This revision is now accepted and ready to land.Jan 14 2017, 12:58 PM
efriedma accepted this revision.Jan 16 2017, 10:25 AM
efriedma edited edge metadata.

Maybe a few of these could be refined a little, but this is a clear improvement.

lib/Analysis/TargetLibraryInfo.cpp
660

Missing check that the third parameter is size_t.

This revision was automatically updated to reflect the committed changes.