[PATCH] D110585: [Analysis][SimplifyLibCalls] Refactor code related to size_t in lib func signatures. NFCI

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D110585#3029588 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:755
     --NumParams;
-    if (!IsSizeTTy(FTy.getParamType(NumParams)))
+    if (!FTy.getParamType(NumParams)->isIntegerTy(SizeTBits))
       return false;
----------------
arichardson wrote:
> nikic wrote:
> > arichardson wrote:
> > > nikic wrote:
> > > > I'd suggest to land these changes as an NFC refactoring -- just determine `SizeTBits` as `DL.getPointerSizeInBits()` for now.
> > > 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?
> > > 
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110585