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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:604
 TLI_DEFINE_STRING_INTERNAL("__sincospi_stret")
+TLI_DEFINE_SIG_INTERNAL(Dbl, Dbl)
+
----------------
These should be marked as manually checked as well.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:44
+enum FuncArgTypeID: char {
+  Void = 0,      // Must be zero.
+  Bool,          // 8 bits on all targets
----------------
msebor wrote:
> nikic wrote:
> > I think it would be better to use a separate zero sentinel value (rather than reusing `Void` for the purpose). It's not like we need to save on enum elements here.
> `Void` is set to zero in order to terminate the argument list, as in:
> `TLI_DEFINE_SIG_INTERNAL(Int)`.
I get that, my point here is that we should do something like `End = 0, Void`, rather than having `Void` both indicate a void return and the end of the list.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:952
+
+bool MatchType(FuncArgTypeID ArgTy, const Type *Ty, unsigned IntBits,
+               unsigned SizeTBits) {
----------------
nikic wrote:
> `static bool matchType`
Function name should start with a lowercase letter (historically the convention was different, that's why you sometimes still see uppercase function names).


================
Comment at: llvm/test/CodeGen/AMDGPU/fabs.ll:79
 define amdgpu_kernel void @fabs_fn_fold(float addrspace(1)* %out, float %in0, float %in1) {
   %fabs = call float @fabs(float %in0)
   %fmul = fmul float %fabs, %in1
----------------
Missed this one.


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

https://reviews.llvm.org/D129915