[PATCH] D129915: [InstCombine] Tighten up known library function signature tests (PR #56463)

[Martin Sebor via Phabricator via llvm-commits]

msebor updated this revision to Diff 447364.
msebor added a comment.
Herald added subscribers: kosarev, pmatos, asb, sstefan1, kerbowa, zzheng, aheejin, jgravelle-google, sbc100, jvesely, dschuff.
Herald added a reviewer: jdoerfert.

Revision 2 of the patch modifies `TargetLibraryInfoImpl::isValidProtoForLibFunc` to use the same consistent approach to validate the vast majority of known library functions.  It replaces the repetitive (verbose and error-prone) argument checking used for groups of functions with a single "table-driven" algorithm.  For better organization it also rearranges the functions by the header they're declared in.  (A further improvement here, to avoid duplication, might be also to arrange them within each header by their number of arguments.)  Finally, this revision is parameterized on the result of `getIntSize()` to correctly validate `int` arguments in 16-bit environments rather than assuming the type is 32 bits everywhere.

My preferred choice of a data structure would have been a constant array to represent the signatures with `LibFunc` as an index to retrieve each in constant time, but this would have required keeping the signatures sorted which feels like too much of a maintenance burden.  With the (hopefully) intuitive notation for the signatures, the large `switch` statement seems like enough of an improvement to readability to avoid the common problems.

I realize this is a large change unlike what @spatel requested.  I was too far along into implementing it when I read the comment.  Although it would have been possible (and still is) to split it up into chunks by progressively moving subsets of cases into a new, ever-growing function, it seems like a lot of intermediate work to get to the same final result.  Based on the test suite failures I ran into while making the changes the coverage here seems pretty good, although there is room for improvement here as well.

A number of existing tests that rely on the uniqueness of distinct pointer types failed with this change because it intentionally doesn't preserve this aspect of the checking.  I've adjusted them to pass, although I'm pretty sure the changes don't exercise the same failure modes as before.  If this is a problem it's easy to preserve it/them until opaque types become the default.

There are a few outstanding gaps that I think should be plugged in a follow-up:

- functions that expect an argument of type `long` successfully match any integer arguments (I couldn't find a `getLongSize()` equivalent of `getIntSize()`),
- functions that expect `long long` assume it's 64 bits on all targets (this may true in practice but it would be nice to parameterize the code nonetheless)
- functions that expect an argument of type `long double` match any floating argument (I didn't want to complicate the logic of matching the corresponding type on each target)
- the special cases for the `cabs` and the `sincospi` families of functions could be generalized and made available to others (such as the `div` functions proposed in D129396 <https://reviews.llvm.org/D129396>).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129915/new/

https://reviews.llvm.org/D129915

Files:
  llvm/lib/Analysis/TargetLibraryInfo.cpp
  llvm/test/CodeGen/AMDGPU/complex-folding.ll
  llvm/test/CodeGen/AMDGPU/fabs.ll
  llvm/test/CodeGen/AMDGPU/floor.ll
  llvm/test/CodeGen/AMDGPU/fneg-fabs.ll
  llvm/test/CodeGen/AMDGPU/r600-infinite-loop-bug-while-reorganizing-vector.ll
  llvm/test/CodeGen/AMDGPU/schedule-if-2.ll
  llvm/test/Transforms/DeadStoreElimination/simple.ll
  llvm/test/Transforms/InferFunctionAttrs/annotate-2.ll
  llvm/test/Transforms/InferFunctionAttrs/annotate.ll
  llvm/test/Transforms/InstCombine/bcopy.ll
  llvm/test/Transforms/InstCombine/deref-alloc-fns.ll
  llvm/test/Transforms/InstCombine/fprintf-1.ll
  llvm/test/Transforms/InstCombine/fwrite-1.ll
  llvm/test/Transforms/InstCombine/mem-deref-bytes.ll
  llvm/test/Transforms/InstCombine/new-delete-itanium-32.ll
  llvm/test/Transforms/InstCombine/new-delete-itanium.ll
  llvm/test/Transforms/InstCombine/printf-2.ll
  llvm/test/Transforms/InstCombine/simplify-libcalls-i16.ll
  llvm/test/Transforms/InstCombine/simplify-libcalls.ll
  llvm/test/Transforms/InstCombine/sprintf-3.ll
  llvm/test/Transforms/InstCombine/sqrt-nofast.ll
  llvm/test/Transforms/InstCombine/stdiocall-bad-sig.ll
  llvm/test/Transforms/InstCombine/stpcpy-2.ll
  llvm/test/Transforms/InstCombine/stpcpy_chk-2.ll
  llvm/test/Transforms/InstCombine/strcall-bad-sig.ll
  llvm/test/Transforms/InstCombine/strcat-3.ll
  llvm/test/Transforms/InstCombine/strcpy-2.ll
  llvm/test/Transforms/InstCombine/strcpy_chk-2.ll
  llvm/test/Transforms/InstCombine/strncat-3.ll
  llvm/test/Transforms/InstCombine/strncpy-2.ll
  llvm/test/Transforms/InstCombine/strncpy_chk-2.ll
  llvm/test/Transforms/InstCombine/strpbrk-2.ll
  llvm/test/Transforms/InstSimplify/call.ll
  llvm/test/Transforms/LoopUnroll/WebAssembly/basic-unrolling.ll
  llvm/test/Transforms/LoopVectorize/AArch64/extractvalue-no-scalarization-required.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129915.447364.patch
Type: text/x-patch
Size: 134613 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220725/96443a11/attachment-0001.bin>