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

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:28
+   (defined(TLI_DEFINE_STRING) ||  defined(TLI_DEFINE_SIG)) ||  \
+   (defined(TLI_DEFINE_STRING) && defined(TLI_DEFINE_SIG))))
+#error "Must define exactly one of TLI_DEFINE_ENUM, TLI_DEFINE_STRING, or TLI_DEFINE_SIG for TLI .def."
----------------
nikic wrote:
> Does the precessor allow writing something like `defined(TLI_DEFINE_ENUM) + defined(TLI_DEFINE_STRING) + defined(TLI_DEFINE_SIG) == 1`? Might be a clearer way to express this...
It sure does.  Good suggestion!


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:913
 TLI_DEFINE_STRING_INTERNAL("cabs")
+TLI_DEFINE_SIG_INTERNAL(Dbl, Dbl)
+
----------------
nikic wrote:
> I'd suggest making this and similar functions `TLI_DEFINE_SIG_INTERNAL(/* Manually checked */)` or so, as the signature specified here will just get ignored...
Sounds good.  Something like that was probably why I had `/* TODO */` for `cabs` below and forgot to get back to it.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:44
+enum FuncArgTypeID: char {
+  Void = 0,      // Must be zero.
+  Bool,          // 8 bits on all targets
----------------
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)`.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1051
+  Type *Ty = FTy.getReturnType(), *LastTy = Ty;
+  auto ProtoTypes = Signatures[F];
+  for (auto TyID: ProtoTypes) {
----------------
nikic wrote:
> `const auto &` (or just drop the var)
I don't mind making it a const reference but the element is an 8-byte struct so making a copy of it should have no overhead, if that's what you're concerned about.


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/annotate.ll:318
+; CHECK: declare noundef i32 @chmod(i8* nocapture noundef readonly, i32 noundef zeroext) [[NOFREE_NOUNWIND]]
+declare i32 @chmod(i8*, i32 zeroext)
 
----------------
nikic wrote:
> It looks like mode_t is actually 16 bits on Darwin. Though I see you already use IntX, so that should still work fine. This test update is probably not needed?
It's not needed but it's wrong for all the other targets.  I initially had `mode_t`, `gid_t`, `uid_t`, and `pid_t` all as `Int` and relaxed it only after I remembered that some kernels use integers of different sizes for some of them.  I'd hope to see the sizes of all these types, as well as `long`, encoded for each target somewhere in `TargetLibraryInfo.cpp` and tested separately for each.  Until that happens I've reverted this and added a comment explaining why `i16` is being used here.


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

https://reviews.llvm.org/D129915