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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Looks great, just some style nits.



================
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."
----------------
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...


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:913
 TLI_DEFINE_STRING_INTERNAL("cabs")
+TLI_DEFINE_SIG_INTERNAL(Dbl, Dbl)
+
----------------
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...


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:43
+// Recognized types of library function arguments and return types.
+enum FuncArgTypeID: char {
+  Void = 0,      // Must be zero.
----------------
I believe the standard formatting has a space before `:`.


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


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:952
+
+bool MatchType(FuncArgTypeID ArgTy, const Type *Ty, unsigned IntBits,
+               unsigned SizeTBits) {
----------------
`static bool matchType`


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1018
+    Type* RetTy = FTy.getReturnType();
+    Type* ParamTy = FTy.getParamType(0);
+    if (auto *Ty = dyn_cast<StructType>(RetTy)) {
----------------
`Type *`


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1029
+        return false;
+      return Ty->getElementType() == ParamTy;
+    }
----------------
I don't think separating one of the conditions out as an early return is really adding something here and would write this as `return Ty->getNumElements() == 2 && T->getElementType() == ParamTy;` Don't feel strongly about this though.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1047
+
+  // Iterate over the type ids in the function prototype , matching each
+  // against the function's type FTy, starting with its return type.
----------------
Stray space before comma


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1051
+  Type *Ty = FTy.getReturnType(), *LastTy = Ty;
+  auto ProtoTypes = Signatures[F];
+  for (auto TyID: ProtoTypes) {
----------------
`const auto &` (or just drop the var)


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1071
+    }
+    else {
+      if (!Ty || !MatchType(TyID, Ty, IntBits, SizeTBits))
----------------
Should be on same line.


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

https://reviews.llvm.org/D129915