[Analysis][SimplifyLibCalls] Refactor code related to size_t in lib func signatures. NFC

[bjope]

Summary

Added a helper in TargetLibraryInfo to get size of "size_t" in bits,
given a Module reference. The new getSizeTSize helper is using the
same strategy as for example isValidProtoForLibFunc has been using
in the past, assuming that the size can be derived by asking
DataLayout about the size/type of a pointer to int.

FortifiedLibCallSimplifier::optimizeStrpCpyChk was changed to use
the new getSizeTSize helper instead of assuming that sizeof(size_t)
is equal to sizeof(int*) by itself (that is the assumption used in
TargetLibraryInfoImpl::getSizeTSize so the result will be the same).

Having a common helper for this ensure that we use the same strategy
when deriving the size of "size_t" in different parts of the code.
One bonus with this refactoring (basing it on Module instead of just
DataLayout) is that it makes it easier to override this for a specific
target triple, in case the assumption of using getPointerSizeInBits
wouldn't hold.

[bjope]

Rebased on top of D110584.

[nikic]

As I mentioned on the other review, I think that DL.getIndexSizeInBits() should be used for the size_t size. If we do that, do we really need a TLI hook?

llvm/lib/Analysis/TargetLibraryInfo.cpp

1004–1005

I'd suggest to land these changes as an NFC refactoring -- just determine SizeTBits as DL.getPointerSizeInBits() for now.

[bjope]

In D110585#3028146, @nikic wrote:

As I mentioned on the other review, I think that DL.getIndexSizeInBits() should be used for the size_t size. If we do that, do we really need a TLI hook?

At least for my target getIndexSize won't match with size_t either. I made a quick experiment with changing the DataLayout to set the index size to something different than pointer size, but that was not so easy because I got lots of failures. At least from LoadStoreVectorizer (Vectorizer::areConsecutivePointers -> Value::stripAndAccumulateConstantOffsets -> assert "The offset bit width does not match the DL specification:"). So it might take some time to fix all such problems (and we've been have IndexSize equals PointerSize but not size of size_t in bits since the beginning of time afaict).

I thought the TLI hook was useful to give this lookup a name and having the logic for how to find size of size_t in one place, regardless. As currently I think we agree that the faulty assumption of getIntPtrType=>"type for size_t" is incorrect, and that assumption has been used in more than one place (and by now it is kind of tricky to find if that has been done in several more places).
If everyone is expected to know about "IndexSize is supposed to be matching size_t", then I agree that the helper just will appear as an unneeded extra wrapper. But I've never seen, or heard, anything about that earlier except in these reviews. I figure that the getSizeTSize hook will be the place where we kind of define/document that relation.

[arichardson]

In D110585#3029472, @bjope wrote:

In D110585#3028146, @nikic wrote:

As I mentioned on the other review, I think that DL.getIndexSizeInBits() should be used for the size_t size. If we do that, do we really need a TLI hook?

At least for my target getIndexSize won't match with size_t either. I made a quick experiment with changing the DataLayout to set the index size to something different than pointer size, but that was not so easy because I got lots of failures. At least from LoadStoreVectorizer (Vectorizer::areConsecutivePointers -> Value::stripAndAccumulateConstantOffsets -> assert "The offset bit width does not match the DL specification:"). So it might take some time to fix all such problems (and we've been have IndexSize equals PointerSize but not size of size_t in bits since the beginning of time afaict).

I thought the TLI hook was useful to give this lookup a name and having the logic for how to find size of size_t in one place, regardless. As currently I think we agree that the faulty assumption of getIntPtrType=>"type for size_t" is incorrect, and that assumption has been used in more than one place (and by now it is kind of tricky to find if that has been done in several more places).
If everyone is expected to know about "IndexSize is supposed to be matching size_t", then I agree that the helper just will appear as an unneeded extra wrapper. But I've never seen, or heard, anything about that earlier except in these reviews. I figure that the getSizeTSize hook will be the place where we kind of define/document that relation.

DL.getIndexSize() should match ptrdiff_t and therefore size_t on pretty much all architectures. In theory these two types could have a different width but I don't know if any such architecture exist(ed). I am not opposed to the hook, but I think it adds unncessary complexity and we should probably just add a comment to DataLayout/LangRef stating that we assume index size matches ptrdiff_t/size_t.

Due to DL.getIntPtrType() -> size_t assumption, I added a really ugly hack for CHERI targets to return IndexType when DL.getIntPtrType() is called with CHERI capabilities: https://github.com/CTSRD-CHERI/llvm-project/blob/bca8a554d2be06b6f8937c5531202a0e3e8d0f9a/llvm/lib/IR/DataLayout.cpp#L857
Ideally we would fix all those calls but we just don't have the time to do so. Considering this hack works, it seems to me that 90+% of DL.getIntPtrType() really want the address range (i.e. ptrdiff_t/size_t) rather than an integer with the same number of bits as void* i.e. (u)intptr_t

@bjope What do your pointers look like? For RISCV-64 CHERI and Arm's Morello we have 128 bits + 1 bit of hidden state, but only 64 of them are used for the address part, so uintptr_t is 128 bits and size_t/ptrdiff_t are 64 bits. Our datalayout string is e-m:e-pf200:128:128:128:64-p:64:64-i64:64-i128:128-n64-S128-A200-P200-G200 (the f in pf200 marks address space 200 as using "fat pointers", since our backend predates non-integral pointers and non-integral pointers also limit optimizations a bit too much since ptrtoint is fine for us). We also have a version CHERI for 32-bit architectures where we 64-bit pointers+32-bit size_t, and therefore use "e-m:e-pf200:64:64:64:32-p:32:32-i64:64-n32-S128-A200-P200-G200"

llvm/lib/Analysis/TargetLibraryInfo.cpp

1004–1005

That would be incorrect for CHERI (well if we hardcode AS0 it will happen to work since we keep AS0 as 64-bit integer pointers and use AS200 for capabilities).
It should IMO be DL.getIndexSizeInBits(DL.getAllocaAddrSpace()) (or getDefaultGlobalsAddressSpace()) since that gives you ptrdiff_t which *should* be compatible with size_t.

One problem we have here is that there is no "libfunc address space" so there is no easy way to determine the expected address space for libfuncs without a pointer argument, etc. Should it be alloca/globals/something new?

[nikic]

llvm/lib/Analysis/TargetLibraryInfo.cpp

1004–1005

To be clear, I agree that this is incorrect, but that would be an NFC refactoring of the existing logic, so we can land the mass refactoring separately from the logic change.

[arichardson]

llvm/lib/Analysis/TargetLibraryInfo.cpp

1004–1005

Sorry if that wasn't clear here - I wasn't advising against the NFC refactoring just giving some context why the current code is wrong for CHERI.

[bjope]

In D110585#3029588, @arichardson wrote:

@bjope What do your pointers look like? For RISCV-64 CHERI and Arm's Morello we have 128 bits + 1 bit of hidden state, but only 64 of them are used for the address part, so uintptr_t is 128 bits and size_t/ptrdiff_t are 64 bits. Our datalayout string is e-m:e-pf200:128:128:128:64-p:64:64-i64:64-i128:128-n64-S128-A200-P200-G200 (the f in pf200 marks address space 200 as using "fat pointers", since our backend predates non-integral pointers and non-integral pointers also limit optimizations a bit too much since ptrtoint is fine for us). We also have a version CHERI for 32-bit architectures where we 64-bit pointers+32-bit size_t, and therefore use "e-m:e-pf200:64:64:64:32-p:32:32-i64:64-n32-S128-A200-P200-G200"

Our data layout is using three different address spaces such as "p:16:16-p1:32:16-p2:32:16". So AS0 is using 16-bit pointers, and AS1 is using 32-bit pointers. And I guess size_t always have been defined as 32 bits (since way back in time, before we started to use LLVM).
But as you can see we do not say anything about "index size", so for AS0 we have PointerSize=16 and IndexSize=16. But size_t is 32 bits, so lib calls etc involving size_t is using i32.

llvm/lib/Analysis/TargetLibraryInfo.cpp

1004–1005

I guess if we shouldn't assume that these libcalls use address space zero for the involved types, then we should validate the address space attributes as well and not only type sizes. So then all this checks that are based on looking an isIntegerTy or isPointerTy etc also need to look at the adress space.
And could there be targets that for example support multiple strlen functions (one for AS0, one for AS1, one for AS2, etc). Not sure how that would be handled. Then one need to loop over all possible address spaces to look for any match? But then I guess SimplifyLibCalls etc might need some updates to support any address spaces a well when optimizing these lib calls.

[bjope]

Rebased.

[arichardson]

llvm/lib/Analysis/TargetLibraryInfo.cpp

1001–1002

I'm not particularly keen on keeping the hardcoded zero in all callers. I am not sure if having different size_t widths depending on the address space is even possible.

Maybe we should just drop the address space argument and use DL.getDefaultGlobalsAS or DL.getAllocaAS (or possibly add a new library-function address space to the datalayout).

1236

no longer hardcodes address space zero, so this comments needs updating.

1240

I think this should be the index size. If that doesn't work for you, you could special-case it based on your target triple?

[bjope]

Rebase and cleanups.

I plan to propose some follow-up patches based on this that make use of this
in more places (BuildLibCalls etc).

[bjope]

llvm/lib/Analysis/TargetLibraryInfo.cpp

1240

My goal here is still just to do an NFC refactoring, to have this logic in one place.

Downstream I'll add a check and an early out based on the target triple, but I'm not that interested in taking the risk of changing the behavior for other targets by using getIndexSize here. But if someone else wants to do that, at least the introduction of this helper has simplified the task as there hopefully only is one place to update.

[nikic]

LGTM

llvm/include/llvm/Analysis/TargetLibraryInfo.h

196

Drop "or 0 is the size is unknown". This size must be known.

llvm/lib/Analysis/TargetLibraryInfo.cpp

1233

triple

[bjope]

llvm/include/llvm/Analysis/TargetLibraryInfo.h

196

Yes, makes sense to drop that. (It was a copy-paste from getWCharSize above, but that one depends on metadata afaik.)

llvm/include/llvm/Analysis/TargetLibraryInfo.h

public:
   void setShouldSignExtI32Param(bool Val) {
     ShouldSignExtI32Param = Val;
   }
 
   /// Returns the size of the wchar_t type in bytes or 0 if the size is unknown.
   /// This queries the 'wchar_size' metadata.
   unsigned getWCharSize(const Module &M) const;
 
+  /// Returns the size of the size_t type in bits.
+  unsigned getSizeTSize(const Module &M) const;
+
   /// Get size of a C-level int or unsigned int, in bits.
   unsigned getIntSize() const {
     return SizeOfInt;
   }
 
   /// Initialize the C-level size of an integer.
   void setIntSize(unsigned Bits) {
     SizeOfInt = Bits;
Attribute::AttrKind getExtAttrForI32Return(bool Signed = true) const {
     return Attribute::None;
   }
 
   /// \copydoc TargetLibraryInfoImpl::getWCharSize()
   unsigned getWCharSize(const Module &M) const {
     return Impl->getWCharSize(M);
   }
 
+  /// \copydoc TargetLibraryInfoImpl::getSizeTSize()
+  unsigned getSizeTSize(const Module &M) const { return Impl->getSizeTSize(M); }
+
   /// \copydoc TargetLibraryInfoImpl::getIntSize()
   unsigned getIntSize() const {
     return Impl->getIntSize();
   }
 
   /// Handle invalidation from the pass manager.
   ///
   /// If we try to invalidate this info, just return false. It cannot become

llvm/lib/Analysis/TargetLibraryInfo.cpp

default:
     break;
   }
 
   llvm_unreachable("Invalid type");
 }
 
 bool TargetLibraryInfoImpl::isValidProtoForLibFunc(const FunctionType &FTy,
                                                    LibFunc F,
                                                    const Module &M) const {
   unsigned NumParams = FTy.getNumParams();
 
   switch (F) {
     // Special handling for <complex.h> functions:
   case LibFunc_cabs:
   case LibFunc_cabsf:
   case LibFunc_cabsl: {
     Type *RetTy = FTy.getReturnType();
     if (!RetTy->isFloatingPointTy())
       return false;
 
     Type *ParamTy = FTy.getParamType(0);
case LibFunc_sincospif_stret: {
 
     return false;
   }
 
   default:
     break;
   }
 
-  // FIXME: There is no guarantee that sizeof(size_t) is equal to
-  // sizeof(int*) for every target. So the assumption used here to derive
-  // the SizeTBits based on the size of an integer pointer in address space
-  // zero isn't always valid.
   unsigned IntBits = getIntSize();
-  unsigned SizeTBits = M.getDataLayout().getPointerSizeInBits(/*AddrSpace=*/0);
+  unsigned SizeTBits = getSizeTSize(M);
   unsigned Idx = 0;
 
   // Iterate over the type ids in the function prototype, matching each
   // against the function's type FTy, starting with its return type.
   // Return true if both match in number and kind, inclduing the ellipsis.
   Type *Ty = FTy.getReturnType(), *LastTy = Ty;
   const auto &ProtoTypes = Signatures[F];
   for (auto TyID : ProtoTypes) {

 
 unsigned TargetLibraryInfoImpl::getWCharSize(const Module &M) const {
   if (auto *ShortWChar = cast_or_null<ConstantAsMetadata>(
       M.getModuleFlag("wchar_size")))
     return cast<ConstantInt>(ShortWChar->getValue())->getZExtValue();
   return 0;
 }
 
+unsigned TargetLibraryInfoImpl::getSizeTSize(const Module &M) const {
+  // There is really no guarantee that sizeof(size_t) is equal to sizeof(int*).
+  // If that isn't true then it should be possible to derive the SizeTTy from
+  // the target triple here instead and do an early return.
+
+  // Historically LLVM assume that size_t has same size as intptr_t (hence
+  // deriving the size from sizeof(int*) in address space zero). This should
+  // work for most targets. For future consideration: DataLayout also implement
+  // getIndexSizeInBits which might map better to size_t compared to
+  // getPointerSizeInBits. Hard coding address space zero here might be
+  // unfortunate as well. Maybe getDefaultGlobalsAddressSpace() or
+  // getAllocaAddrSpace() is better.
+  unsigned AddressSpace = 0;
+  return M.getDataLayout().getPointerSizeInBits(AddressSpace);
+}
+
 TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass()
     : ImmutablePass(ID), TLA(TargetLibraryInfoImpl()) {
   initializeTargetLibraryInfoWrapperPassPass(*PassRegistry::getPassRegistry());
 }
 
 TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass(const Triple &T)
     : ImmutablePass(ID), TLA(TargetLibraryInfoImpl(T)) {
   initializeTargetLibraryInfoWrapperPassPass(*PassRegistry::getPassRegistry());

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *FortifiedLibCallSimplifier::optimizeStrpCpyChk(CallInst *CI,
 
   // Maybe we can stil fold __st[rp]cpy_chk to __memcpy_chk.
   uint64_t Len = GetStringLength(Src);
   if (Len)
     annotateDereferenceableBytes(CI, 1, Len);
   else
     return nullptr;
 
-  // 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
-  // SizeTBits based on the size of an integer pointer in address space zero
-  // isn't always valid.
-  Type *SizeTTy = DL.getIntPtrType(CI->getContext(), /*AddressSpace=*/0);
+  unsigned SizeTBits = TLI->getSizeTSize(*CI->getModule());
+  Type *SizeTTy = IntegerType::get(CI->getContext(), SizeTBits);
   Value *LenV = ConstantInt::get(SizeTTy, Len);
   Value *Ret = emitMemCpyChk(Dst, Src, LenV, ObjSize, B, DL, TLI);
   // If the function was an __stpcpy_chk, and we were able to fold it into
   // a __memcpy_chk, we still need to return the correct end pointer.
   if (Ret && Func == LibFunc_stpcpy_chk)
     return B.CreateInBoundsGEP(B.getInt8Ty(), Dst,
                                ConstantInt::get(SizeTTy, Len - 1));
   return copyFlags(*CI, cast<CallInst>(Ret));