[SLC] Simplify strcpy and friends with non-zero address spaces

[arichardson]

Summary

[SLC] Simplify strcpy and friends with non-zero address spaces

The current logic in TargetLibraryInfoImpl::getLibFunc() was only treating
strcpy, etc. with i8* arguments in address space zero as a valid library
function. However, in the CHERI and Morello targets we expect all libc
functions to use address space 200 arguments.

This commit updates isValidProtoForLibFunc() to check that the argument
is a pointer type. This also drops the check for i8* since we should not
be checking the pointee type any more.

Depends on D95138

[arsenm]

llvm/lib/Analysis/TargetLibraryInfo.cpp

647–649

This wouldn't really be right for AMDGPU, but we have no library calls anyway. I also think it's not right to use GlobalsAddressSpace for this purpose. This has nothing to do with globals or creating new values

650

Should never depend on pointee element type

[jrtc27]

llvm/lib/Analysis/TargetLibraryInfo.cpp

647–649

The issue is there's no notion of "the address space that libfuncs use". The additional granularity for globals vs stack (vs others too maybe?) is unhelpful here; for us they'll all be the same (AS 200), but there is no sensible mapping from C language void * (or, in this case, char *) to the AS in use.

650

But then it's not a char pointer?

[arichardson]

llvm/lib/Analysis/TargetLibraryInfo.cpp

647–649

More than happy to drop this restriction and just accept any i8*.

I guess something like a "malloc/heap" address space would be more accurate since it's used when checking the realloc return value. It doesn't really matter too much for CHERI since we use 200 for program+alloca+globals.

650

I know this will cause issues in the future, but I wasn't sure how else I can check for i8* ignoring the addres space.

What is the recommended way to avoid this? Or should I just check for ->isPointerType()?

[arsenm]

llvm/lib/Analysis/TargetLibraryInfo.cpp

647–649

I believe the current precedent assumes libcalls must use address space 0

650

The pointee type is meaningless in the IR and will be going away. You can just ignore it

[arichardson]

llvm/lib/Analysis/TargetLibraryInfo.cpp

647–649

Unfortunately we can't use AS0 for CHERI due to compatibility with with MIPS/RISC-V/AArch64.

Would you be okay with me replacing the == PCharTy with ->isPointerType()? That would allow us to simplify these libcalls and also doesn't depend on the pointee type any more.

[arsenm]

llvm/lib/Analysis/TargetLibraryInfo.cpp

647–649

Yes, only being a pointer matters

[arichardson]

Use isPointerType() instead.

[arichardson]

"fix" failing strncpy_chk-2.ll

[xbolva00]

llvm/test/Transforms/InstCombine/strcpy-nonzero-as.ll

40

Remove TODOs

[arichardson]

Drop resolved TODO comments

[arichardson]

rebase after latest changes to TLI

[arichardson]

rebase. Is this okay to land now?

[arichardson]

ping?

[mdchen]

The pointee type is meaningless in the IR and will be going away.

@arsenm @arichardson Hi, this change let user-defined string functions(e.g., size_t strlen(int*)) pass the check and considered a libfunc, which can cause failure in later optimizations like optimizeStrLen which assumes CharSize is 8. And pointed type does matter in this case. What do you think? (This patch doesn't cause a specific bug but I have doubt about the direction towards)

[jrtc27]

In D95142#3157370, @mdchen wrote:

The pointee type is meaningless in the IR and will be going away.

@arsenm @arichardson Hi, this change let user-defined string functions(e.g., size_t strlen(int*)) pass the check and considered a libfunc, which can cause failure in later optimizations like optimizeStrLen which assumes CharSize is 8. And pointed type does matter in this case. What do you think?

The code should've been compiled with either -fno-builtin-strlen or -ffreestanding, or used the right type for strlen, anything else is invalid. See -Wincompatible-library-redeclaration.

[mdchen]

In D95142#3157371, @jrtc27 wrote:

In D95142#3157370, @mdchen wrote:

The pointee type is meaningless in the IR and will be going away.

@arsenm @arichardson Hi, this change let user-defined string functions(e.g., size_t strlen(int*)) pass the check and considered a libfunc, which can cause failure in later optimizations like optimizeStrLen which assumes CharSize is 8. And pointed type does matter in this case. What do you think?

The code should've been compiled with either -fno-builtin-strlen or -ffreestanding, or used the right type for strlen, anything else is invalid. See -Wincompatible-library-redeclaration.

Oh I see, thanks for pointing it out!

llvm/lib/Analysis/TargetLibraryInfo.cpp

bool TargetLibraryInfoImpl::getLibFunc(StringRef funcName, LibFunc &F) const {
   }
   return false;
 }
 
 bool TargetLibraryInfoImpl::isValidProtoForLibFunc(const FunctionType &FTy,
                                                    LibFunc F,
                                                    const DataLayout *DL) const {
   LLVMContext &Ctx = FTy.getContext();
-  Type *PCharTy = Type::getInt8PtrTy(Ctx);
   Type *SizeTTy = DL ? DL->getIntPtrType(Ctx, /*AddressSpace=*/0) : nullptr;
   auto IsSizeTTy = [SizeTTy](Type *Ty) {
     return SizeTTy ? Ty == SizeTTy : Ty->isIntegerTy();
   };
   unsigned NumParams = FTy.getNumParams();
 
   switch (F) {
   case LibFunc_execl:
   case LibFunc_execlp:
   case LibFunc_execle:
     return (NumParams >= 2 && FTy.getParamType(0)->isPointerTy() &&
             FTy.getParamType(1)->isPointerTy() &&
             FTy.getReturnType()->isIntegerTy(32));
   case LibFunc_execv:
   case LibFunc_execvp:
     return (NumParams == 2 && FTy.getParamType(0)->isPointerTy() &&
             FTy.getParamType(1)->isPointerTy() &&
             FTy.getReturnType()->isIntegerTy(32));
   case LibFunc_execvP:
case LibFunc_stpcpy_chk:
     --NumParams;
     if (!IsSizeTTy(FTy.getParamType(NumParams)))
       return false;
     LLVM_FALLTHROUGH;
   case LibFunc_strcpy:
   case LibFunc_stpcpy:
     return (NumParams == 2 && FTy.getReturnType() == FTy.getParamType(0) &&
             FTy.getParamType(0) == FTy.getParamType(1) &&
-            FTy.getParamType(0) == PCharTy);
+            FTy.getParamType(0)->isPointerTy());
 
   case LibFunc_strlcat_chk:
   case LibFunc_strlcpy_chk:
     --NumParams;
     if (!IsSizeTTy(FTy.getParamType(NumParams)))
       return false;
     LLVM_FALLTHROUGH;
   case LibFunc_strlcat:
   case LibFunc_strlcpy:
     return NumParams == 3 && IsSizeTTy(FTy.getReturnType()) &&
            FTy.getParamType(0)->isPointerTy() &&
            FTy.getParamType(1)->isPointerTy() &&
            IsSizeTTy(FTy.getParamType(2));
 
   case LibFunc_strncpy_chk:
   case LibFunc_stpncpy_chk:
     --NumParams;
     if (!IsSizeTTy(FTy.getParamType(NumParams)))
       return false;
     LLVM_FALLTHROUGH;
   case LibFunc_strncpy:
   case LibFunc_stpncpy:
     return (NumParams == 3 && FTy.getReturnType() == FTy.getParamType(0) &&
             FTy.getParamType(0) == FTy.getParamType(1) &&
-            FTy.getParamType(0) == PCharTy &&
+            FTy.getParamType(0)->isPointerTy() &&
             IsSizeTTy(FTy.getParamType(2)));
 
   case LibFunc_strxfrm:
     return (NumParams == 3 && FTy.getParamType(0)->isPointerTy() &&
             FTy.getParamType(1)->isPointerTy());
 
   case LibFunc_strcmp:
     return (NumParams == 2 && FTy.getReturnType()->isIntegerTy(32) &&
if (!IsSizeTTy(FTy.getParamType(NumParams)))
       return false;
     LLVM_FALLTHROUGH;
   case LibFunc_memccpy:
     return (NumParams >= 2 && FTy.getParamType(1)->isPointerTy());
   case LibFunc_memalign:
     return (FTy.getReturnType()->isPointerTy());
   case LibFunc_realloc:
   case LibFunc_reallocf:
-    return (NumParams == 2 && FTy.getReturnType() == PCharTy &&
+    return (NumParams == 2 && FTy.getReturnType()->isPointerTy() &&
             FTy.getParamType(0) == FTy.getReturnType() &&
             IsSizeTTy(FTy.getParamType(1)));
   case LibFunc_read:
     return (NumParams == 3 && FTy.getParamType(1)->isPointerTy());
   case LibFunc_rewind:
   case LibFunc_rmdir:
   case LibFunc_remove:
   case LibFunc_realpath:
return (NumParams >= 2 && FTy.getReturnType()->isIntegerTy() &&
             FTy.getParamType(1)->isPointerTy());
   case LibFunc_fgetpos:
     return (NumParams >= 2 && FTy.getParamType(0)->isPointerTy() &&
             FTy.getParamType(1)->isPointerTy());
   case LibFunc_getchar:
   case LibFunc_getchar_unlocked:
     return (NumParams == 0 && FTy.getReturnType()->isIntegerTy());
   case LibFunc_gets:
-    return (NumParams == 1 && FTy.getParamType(0) == PCharTy);
+    return (NumParams == 1 && FTy.getParamType(0)->isPointerTy());
   case LibFunc_getitimer:
     return (NumParams == 2 && FTy.getParamType(1)->isPointerTy());
   case LibFunc_ungetc:
     return (NumParams == 2 && FTy.getParamType(1)->isPointerTy());
   case LibFunc_utime:
   case LibFunc_utimes:
     return (NumParams == 2 && FTy.getParamType(0)->isPointerTy() &&
             FTy.getParamType(1)->isPointerTy());
bool TargetLibraryInfoImpl::isValidProtoForLibFunc(const FunctionType &FTy,
 
   case LibFunc_sinpif:
   case LibFunc_cospif:
     return (NumParams == 1 && FTy.getReturnType()->isFloatTy() &&
             FTy.getReturnType() == FTy.getParamType(0));
 
   case LibFunc_strnlen:
     return (NumParams == 2 && FTy.getReturnType() == FTy.getParamType(1) &&
-            FTy.getParamType(0) == PCharTy &&
-            FTy.getParamType(1) == SizeTTy);
+            FTy.getParamType(0)->isPointerTy() &&
+            IsSizeTTy(FTy.getParamType(1)));
 
   case LibFunc_posix_memalign:
     return (NumParams == 3 && FTy.getReturnType()->isIntegerTy(32) &&
             FTy.getParamType(0)->isPointerTy() &&
-            FTy.getParamType(1) == SizeTTy && FTy.getParamType(2) == SizeTTy);
+            IsSizeTTy(FTy.getParamType(1)) && IsSizeTTy(FTy.getParamType(2)));
 
   case LibFunc_wcslen:
     return (NumParams == 1 && FTy.getParamType(0)->isPointerTy() &&
             FTy.getReturnType()->isIntegerTy());
 
   case LibFunc_cabs:
   case LibFunc_cabsf:
   case LibFunc_cabsl: {

llvm/test/Transforms/InstCombine/strcpy-nonzero-as.ll

 declare i8 addrspace(200)* @strncpy(i8 addrspace(200)*, i8 addrspace(200)*, i64) addrspace(200)
 declare i8 addrspace(200)* @stpncpy(i8 addrspace(200)*, i8 addrspace(200)*, i64) addrspace(200)
 
 ; TODO: this should be converted to a memcpy
 define void @test_strcpy_to_memcpy(i8 addrspace(200)* %dst) addrspace(200) nounwind {
 ; CHECK-LABEL: define {{[^@]+}}@test_strcpy_to_memcpy
 ; CHECK-SAME: (i8 addrspace(200)* [[DST:%.*]]) addrspace(200) [[ATTR0:#.*]] {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[CALL:%.*]] = call addrspace(200) i8 addrspace(200)* @strcpy(i8 addrspace(200)* [[DST]], i8 addrspace(200)* getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0))
+; CHECK-NEXT:    call addrspace(200) void @llvm.memcpy.p200i8.p200i8.i64(i8 addrspace(200)* align 1 dereferenceable(17) [[DST]], i8 addrspace(200)* align 1 dereferenceable(17) getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0), i64 17, i1 false)
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %call = call i8 addrspace(200)* @strcpy(i8 addrspace(200)* %dst, i8 addrspace(200)* getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0))
   ret void
 }
 
 ; TODO: this should be converted to a memcpy
 define void @test_stpcpy_to_memcpy(i8 addrspace(200)* %dst) addrspace(200) nounwind {
 ; CHECK-LABEL: define {{[^@]+}}@test_stpcpy_to_memcpy
 ; CHECK-SAME: (i8 addrspace(200)* [[DST:%.*]]) addrspace(200) [[ATTR0]] {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[CALL:%.*]] = call addrspace(200) i8 addrspace(200)* @stpcpy(i8 addrspace(200)* [[DST]], i8 addrspace(200)* getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0))
+; CHECK-NEXT:    call addrspace(200) void @llvm.memcpy.p200i8.p200i8.i128(i8 addrspace(200)* align 1 dereferenceable(17) [[DST]], i8 addrspace(200)* align 1 dereferenceable(17) getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0), i128 17, i1 false)
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %call = call i8 addrspace(200)* @stpcpy(i8 addrspace(200)* %dst, i8 addrspace(200)* getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0))
   ret void
 }
 
 ; TODO: this should be converted to a memcpy
 define void @test_strncpy_to_memcpy(i8 addrspace(200)* %dst) addrspace(200) nounwind {
 ; CHECK-LABEL: define {{[^@]+}}@test_strncpy_to_memcpy
 ; CHECK-SAME: (i8 addrspace(200)* [[DST:%.*]]) addrspace(200) [[ATTR0]] {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[CALL:%.*]] = call addrspace(200) i8 addrspace(200)* @strncpy(i8 addrspace(200)* [[DST]], i8 addrspace(200)* getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0), i64 17)
+; CHECK-NEXT:    call addrspace(200) void @llvm.memcpy.p200i8.p200i8.i128(i8 addrspace(200)* align 1 dereferenceable(17) [[DST]], i8 addrspace(200)* align 1 dereferenceable(17) getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0), i128 17, i1 false)
 ; CHECK-NEXT:    ret void
 ;
 entry:
   %call = call i8 addrspace(200)* @strncpy(i8 addrspace(200)* %dst, i8 addrspace(200)* getelementptr inbounds ([17 x i8], [17 x i8] addrspace(200)* @str, i64 0, i64 0), i64 17)
   ret void
 }
 
 ; Note: stpncpy is not handled by SimplifyLibcalls yet, so this should not be changed.

llvm/test/Transforms/InstCombine/strncpy_chk-2.ll

 ; Test that lib call simplification doesn't simplify __strncpy_chk calls
 ; with the wrong prototype.
 ;
 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128"
 
 @a = common global [60 x i16] zeroinitializer, align 1
-@b = common global [60 x i16] zeroinitializer, align 1
+@b = common global [60 x i8] zeroinitializer, align 1
 
 define void @test_no_simplify() {
 ; CHECK-LABEL: @test_no_simplify(
   %dst = getelementptr inbounds [60 x i16], [60 x i16]* @a, i32 0, i32 0
-  %src = getelementptr inbounds [60 x i16], [60 x i16]* @b, i32 0, i32 0
+  %src = getelementptr inbounds [60 x i8], [60 x i8]* @b, i32 0, i32 0
 
 ; CHECK-NEXT: call i16* @__strncpy_chk
-  call i16* @__strncpy_chk(i16* %dst, i16* %src, i32 60, i32 60)
+  call i16* @__strncpy_chk(i16* %dst, i8* %src, i32 60, i32 60)
   ret void
 }
 
-declare i16* @__strncpy_chk(i16*, i16*, i32, i32)
+declare i16* @__strncpy_chk(i16*, i8*, i32, i32)