[Analysis] Be defensive when matching size_t in lib call signatures

[bjope]

Summary

When TargetLibraryInfoImpl::isValidProtoForLibFunc is checking
function signatures to detect lib calls it may check that a parameter
or return value matches with the "size_t" type. For this to work it
has to derive the IR type matching with "size_t". Depending on if
a DataLayout is provided or not, this has been done in two different
way. Either a more strict check being based on IntPtrType (which is
given by the DataLayout) or a more relaxed check assuming that any
integer type matches with "size_t".

Given that the stricter approach exist it seems like we do not want
to trigger rewrites etc if we aren't sure that a function calls
actually match with the library function. Therefore it was questioned
why we actually have the more relaxed approach when not being able
to derive an IR type for "size_t". This patch will take a more
defensive approach, avoiding lib call transformations when we do not
know if the signature matches (when we do not know the size of
"size_t").

[nikic]

I think the "optional DataLayout" part here is just an unused leftover. If I add an assert(DL) in getLibFunc() (the only caller of isValidProtoForLibFunc()) I don't get any test failures. I'd recommend just making DataLayout explicitly required. Assert / make it a reference.

[nikic]

llvm/lib/Analysis/TargetLibraryInfo.cpp

732

As a side note, I believe this should really be using the integer type with width DL.getIndexSizeInBits(). Basically pointer size = intptr_t and index size = size_t.

[bjope]

llvm/lib/Analysis/TargetLibraryInfo.cpp

732

Ok. I did not know about "index size = size_t" relation.

(Makes me wonder about the index type in the DataLayout for our OOT target. We haven't configured that afaict, so we get index size = pointer size by default. But in our target size_t is different from intptr_t. So we should probably specify those optional params in the data layout to set the IndexSize. Will be interesting to see if that will impact something else.)

[bjope]

Update after review feedback:
Now assuming that we always have a Module (and DataLayout) when using getLibFunc/isValidProtoForLibFunc.

[nikic]

LG

llvm/include/llvm/Analysis/TargetLibraryInfo.h

class TargetLibraryInfoImpl {
   std::vector<VecDesc> VectorDescs;
   /// Scalarization descriptors - same content as VectorDescs but sorted based
   /// on VectorFnName rather than ScalarFnName.
   std::vector<VecDesc> ScalarDescs;
 
   /// Return true if the function type FTy is valid for the library function
   /// F, regardless of whether the function is available.
   bool isValidProtoForLibFunc(const FunctionType &FTy, LibFunc F,
-                              const DataLayout *DL) const;
+                              const DataLayout &DL) const;
 
 public:
   /// List of known vector-functions libraries.
   ///
   /// The vector-functions library defines, which functions are vectorizable
   /// and with which factor. The library can be specified by either frontend,
   /// or a commandline option, and then used by
   /// addVectorizableFunctionsFromVecLib for filling up the tables of
public:
   /// corresponding value.
   bool getLibFunc(StringRef funcName, LibFunc &F) const;
 
   /// Searches for a particular function name, also checking that its type is
   /// valid for the library function matching that name.
   ///
   /// If it is one of the known library functions, return true and set F to the
   /// corresponding value.
+  ///
+  /// FDecl is assumed to have a parent Module when using this function.
   bool getLibFunc(const Function &FDecl, LibFunc &F) const;
 
   /// Forces a function to be marked as unavailable.
   void setUnavailable(LibFunc F) {
     setState(F, Unavailable);
   }
 
   /// Forces a function to be marked as available.

llvm/lib/Analysis/TargetLibraryInfo.cpp

if (I != End && *I == funcName) {
     F = (LibFunc)(I - Start);
     return true;
   }
   return false;
 }
 
 bool TargetLibraryInfoImpl::isValidProtoForLibFunc(const FunctionType &FTy,
                                                    LibFunc F,
-                                                   const DataLayout *DL) const {
+                                                   const DataLayout &DL) const {
   LLVMContext &Ctx = FTy.getContext();
   // FIXME: There is really no guarantee that sizeof(size_t) is equal to
   // sizeof(int*) for every target. So the assumption used here to derive the
   // SizeTTy based on DataLayout and getIntPtrType isn't always valid.
-  Type *SizeTTy = DL ? DL->getIntPtrType(Ctx, /*AddressSpace=*/0) : nullptr;
+  Type *SizeTTy = DL.getIntPtrType(Ctx, /*AddressSpace=*/0);
   auto IsSizeTTy = [SizeTTy](Type *Ty) {
-    // FIXME: For uknown historical reasons(?) we use a relaxed condition saying
-    // that any integer type may size_t, for example if we got no
-    // DataLayout. This seems like a potentially error prone relaxation (or why
-    // should we only be more strict and checking the exact type when we have a
-    // DataLayout?).
-    if (!SizeTTy)
-      return Ty->isIntegerTy();
     return Ty == SizeTTy;
   };
   unsigned NumParams = FTy.getNumParams();
 
   switch (F) {
   case LibFunc_execl:
   case LibFunc_execlp:
   case LibFunc_execle:

 
 bool TargetLibraryInfoImpl::getLibFunc(const Function &FDecl,
                                        LibFunc &F) const {
   // Intrinsics don't overlap w/libcalls; if our module has a large number of
   // intrinsics, this ends up being an interesting compile time win since we
   // avoid string normalization and comparison.
   if (FDecl.isIntrinsic()) return false;
 
-  const DataLayout *DL =
-      FDecl.getParent() ? &FDecl.getParent()->getDataLayout() : nullptr;
+  const Module *M = FDecl.getParent();
+  assert(M && "Expecting FDecl to be connected to a Module.");
+
   return getLibFunc(FDecl.getName(), F) &&
-         isValidProtoForLibFunc(*FDecl.getFunctionType(), F, DL);
+         isValidProtoForLibFunc(*FDecl.getFunctionType(), F, M->getDataLayout());
 }
 
 void TargetLibraryInfoImpl::disableAllFunctions() {
   memset(AvailableArray, 0, sizeof(AvailableArray));
 }
 
 static bool compareByScalarFnName(const VecDesc &LHS, const VecDesc &RHS) {
   return LHS.ScalarFnName < RHS.ScalarFnName;