[InferAttrs] Improve DSE for libcalls (partial fix for PR47644)

[xbolva00]

Summary

Optimize `
void f(const char *s) {

char a[256];
__builtin_strcpy(a, s);  // dead store

}

to just empty function.

[xbolva00]

Interestingly, as we can see, something is broken with NPM (hopefully not a blocker for this patch)

[fhahn]

I think it would be good to update the commit message to just state that this adds no capture and write only to the first arg of strcpy & co. It also unifies strcpy & stpcpy handling. might be good to do separately first.

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

11

Can we just add those tests to llvm/test/Transforms/DeadStoreElimination/MSSA/libcalls.ll?

[fhahn]

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

19

I think the NPM would require -aa-pipeline=basic-aa. But given that the interaction between -infer-attrs/-dse is very small, I think it would be best to move the checks to the existing lib calls.ll

[xbolva00]

In D88328#2295737, @fhahn wrote:

I think it would be good to update the commit message to just state that this adds no capture and write only to the first arg of strcpy & co. It also unifies strcpy & stpcpy handling. might be good to do separately first.

Ok.

So first step: https://reviews.llvm.org/D88335

[jdoerfert]

Needs a rebase.

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

13

I'm unsure how this is optimized because I don't see argmemonly on the strcpy. Did I miss it? If not, what is the justification here?

[xbolva00]

Ok, I should be ready to make progress here again.

@fhahn, with manually added writeonly, this optimization no longer works for me somehow.

https://godbolt.org/z/jErj3f

Any recent change in MSSA DSE which could cause this behaviour?

[xbolva00]

Test cases work for me with clang/opt 11 with mssa dse

https://godbolt.org/z/648nMq

https://godbolt.org/z/qPaMME

[fhahn]

In D88328#2337212, @xbolva00 wrote:

Test cases work for me with clang/opt 11 with mssa dse

https://godbolt.org/z/648nMq

https://godbolt.org/z/qPaMME

I think there was a change how lifetime.end is handled in MemorySSA. Adjusted DSE in f5cf7f544b7abe8488f76945537044f700b5548a.

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

13

I think this is due to an inconsistency with the attributes. The implicit assumption is that MemoryLocation::getForArgument currently only returns a proper location for argmemonly I think. We should mark them as argmemonly and then probably have an assert in getLocationFor that is is only used for such functions.

[xbolva00]

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

13

We should mark them as argmemonly

This part is done (I committed it yesterday)

[xbolva00]

Thanks for fast fix!

[fhahn]

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

13

nice, so I guess it should be safe to add the assert now.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

 //- Infer Attributes ---------------------------------------------------------//
 
 STATISTIC(NumReadNone, "Number of functions inferred as readnone");
 STATISTIC(NumReadOnly, "Number of functions inferred as readonly");
 STATISTIC(NumArgMemOnly, "Number of functions inferred as argmemonly");
 STATISTIC(NumNoUnwind, "Number of functions inferred as nounwind");
 STATISTIC(NumNoCapture, "Number of arguments inferred as nocapture");
 STATISTIC(NumReadOnlyArg, "Number of arguments inferred as readonly");
+STATISTIC(NumWriteOnlyArg, "Number of arguments inferred as writeonly");
 STATISTIC(NumNoAlias, "Number of function returns inferred as noalias");
 STATISTIC(NumNoUndef, "Number of function returns inferred as noundef returns");
 STATISTIC(NumNonNull, "Number of function returns inferred as nonnull returns");
 STATISTIC(NumReturnedArg, "Number of arguments inferred as returned");
 
 static bool setDoesNotAccessMemory(Function &F) {
   if (F.doesNotAccessMemory())
     return false;

 static bool setOnlyReadsMemory(Function &F, unsigned ArgNo) {
   if (F.hasParamAttribute(ArgNo, Attribute::ReadOnly))
     return false;
   F.addParamAttr(ArgNo, Attribute::ReadOnly);
   ++NumReadOnlyArg;
   return true;
 }
 
+static bool setOnlyWritesMemory(Function &F, unsigned ArgNo) {
+  if (F.hasParamAttribute(ArgNo, Attribute::WriteOnly))
+    return false;
+  F.addParamAttr(ArgNo, Attribute::WriteOnly);
+  ++NumWriteOnlyArg;
+  return true;
+}
+
 static bool setRetAndArgsNoUndef(Function &F) {
   bool Changed = false;
   if (!F.getReturnType()->isVoidTy() &&
       !F.hasAttribute(AttributeList::ReturnIndex, Attribute::NoUndef)) {
     F.addAttribute(AttributeList::ReturnIndex, Attribute::NoUndef);
     ++NumNoUndef;
     Changed = true;
   }
bool llvm::inferLibFuncAttributes(Function &F, const TargetLibraryInfo &TLI) {
   case LibFunc_strtold:
   case LibFunc_strtoull:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 0);
     return Changed;
   case LibFunc_strcpy:
   case LibFunc_strncpy:
-    Changed |= setDoesNotAlias(F, 0);
-    Changed |= setDoesNotAlias(F, 1);
-    LLVM_FALLTHROUGH;
   case LibFunc_strcat:
   case LibFunc_strncat:
     Changed |= setReturnedArg(F, 0);
     LLVM_FALLTHROUGH;
   case LibFunc_stpcpy:
   case LibFunc_stpncpy:
     Changed |= setDoesNotThrow(F);
+    Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
+    Changed |= setOnlyWritesMemory(F, 0);
     Changed |= setOnlyReadsMemory(F, 1);
+    Changed |= setDoesNotAlias(F, 0);
+    Changed |= setDoesNotAlias(F, 1);
     return Changed;
   case LibFunc_strxfrm:
     Changed |= setDoesNotThrow(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_strcmp:      // 0,1

llvm/test/Transforms/InferFunctionAttrs/annotate.ll

 declare i32 @stat64(i8*, %opaque*)
 
 ; CHECK: declare noundef i32 @statvfs(i8* nocapture noundef readonly, %opaque* nocapture noundef) [[G1]]
 declare i32 @statvfs(i8*, %opaque*)
 
 ; CHECK-LINUX: declare noundef i32 @statvfs64(i8* nocapture noundef readonly, %opaque* nocapture noundef) [[G1]]
 declare i32 @statvfs64(i8*, %opaque*)
 
-; CHECK: declare i8* @stpcpy(i8*, i8* nocapture readonly) [[G1]]
+; CHECK: declare i8* @stpcpy(i8* noalias nocapture writeonly, i8* noalias nocapture readonly) [[G1]]
 declare i8* @stpcpy(i8*, i8*)
 
-; CHECK: declare i8* @stpncpy(i8*, i8* nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @stpncpy(i8* noalias nocapture writeonly, i8* noalias nocapture readonly, i64) [[G1]]
 declare i8* @stpncpy(i8*, i8*, i64)
 
 ; CHECK: declare i32 @strcasecmp(i8* nocapture, i8* nocapture) [[G2]]
 declare i32 @strcasecmp(i8*, i8*)
 
-; CHECK: declare i8* @strcat(i8* returned, i8* nocapture readonly) [[G1]]
+; CHECK: declare i8* @strcat(i8* noalias nocapture returned writeonly, i8* noalias nocapture readonly) [[G1]]
 declare i8* @strcat(i8*, i8*)
 
 ; CHECK: declare i8* @strchr(i8*, i32) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i8* @strchr(i8*, i32)
 
 ; CHECK: declare i32 @strcmp(i8* nocapture, i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i32 @strcmp(i8*, i8*)
 
 ; CHECK: declare i32 @strcoll(i8* nocapture, i8* nocapture) [[G2]]
 declare i32 @strcoll(i8*, i8*)
 
-; CHECK: declare i8* @strcpy(i8* noalias returned, i8* noalias nocapture readonly) [[G1]]
+; CHECK: declare i8* @strcpy(i8* noalias nocapture returned writeonly, i8* noalias nocapture readonly) [[G1]]
 declare i8* @strcpy(i8*, i8*)
 
 ; CHECK: declare i64 @strcspn(i8* nocapture, i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i64 @strcspn(i8*, i8*)
 
 ; CHECK: declare noalias i8* @strdup(i8* nocapture readonly) [[G1]]
 declare i8* @strdup(i8*)
 
 ; CHECK: declare i64 @strlen(i8* nocapture) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i64 @strlen(i8*)
 
 ; CHECK: declare i32 @strncasecmp(i8* nocapture, i8* nocapture, i64) [[G2]]
 declare i32 @strncasecmp(i8*, i8*, i64)
 
-; CHECK: declare i8* @strncat(i8* returned, i8* nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @strncat(i8* noalias nocapture returned writeonly, i8* noalias nocapture readonly, i64) [[G1]]
 declare i8* @strncat(i8*, i8*, i64)
 
 ; CHECK: declare i32 @strncmp(i8* nocapture, i8* nocapture, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 declare i32 @strncmp(i8*, i8*, i64)
 
-; CHECK: declare i8* @strncpy(i8* noalias returned, i8* noalias nocapture readonly, i64) [[G1]]
+; CHECK: declare i8* @strncpy(i8* noalias nocapture returned writeonly, i8* noalias nocapture readonly, i64) [[G1]]
 declare i8* @strncpy(i8*, i8*, i64)
 
 ; CHECK: declare noalias i8* @strndup(i8* nocapture readonly, i64) [[G1]]
 declare i8* @strndup(i8*, i64)
 
 ; CHECK: declare i64 @strnlen(i8*, i64) [[G0]]
 declare i64 @strnlen(i8*, i64)
 

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -S -O2 | FileCheck %s --check-prefixes=CHECK,OLDPM
+; RUN: opt -passes='default<O2>' -S < %s | FileCheck %s --check-prefixes=CHECK,NEWPM
+
+
+declare i8* @strcpy(i8*, i8*)
+declare i8* @strncpy(i8*, i8*, i64)
+declare i8* @strcat(i8*, i8*)
+declare i8* @strncat(i8*, i8*, i64)
+
+define void @dse_strcpy(i8* nocapture readonly %src) {
+; OLDPM-LABEL: @dse_strcpy(
+; OLDPM-NEXT:    ret void
+;
+; NEWPM-LABEL: @dse_strcpy(
+; NEWPM-NEXT:    [[A:%.*]] = alloca [256 x i8], align 16
+; NEWPM-NEXT:    [[BUF:%.*]] = getelementptr inbounds [256 x i8], [256 x i8]* [[A]], i64 0, i64 0
+; NEWPM-NEXT:    call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    [[TMP1:%.*]] = call i8* @strcpy(i8* nonnull [[BUF]], i8* nonnull dereferenceable(1) [[SRC:%.*]])
+; NEWPM-NEXT:    call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    ret void
+;
+  %a = alloca [256 x i8], align 16
+  %buf = getelementptr inbounds [256 x i8], [256 x i8]* %a, i64 0, i64 0
+  call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull %buf)
+  call i8* @strcpy(i8* nonnull %buf, i8* nonnull dereferenceable(1) %src)
+  call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull %buf)
+  ret void
+}
+
+define void @dse_strncpy(i8* nocapture readonly %src) {
+; OLDPM-LABEL: @dse_strncpy(
+; OLDPM-NEXT:    ret void
+;
+; NEWPM-LABEL: @dse_strncpy(
+; NEWPM-NEXT:    [[A:%.*]] = alloca [256 x i8], align 16
+; NEWPM-NEXT:    [[BUF:%.*]] = getelementptr inbounds [256 x i8], [256 x i8]* [[A]], i64 0, i64 0
+; NEWPM-NEXT:    call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    [[TMP1:%.*]] = call i8* @strncpy(i8* nonnull [[BUF]], i8* nonnull dereferenceable(1) [[SRC:%.*]], i64 6)
+; NEWPM-NEXT:    call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    ret void
+;
+  %a = alloca [256 x i8], align 16
+  %buf = getelementptr inbounds [256 x i8], [256 x i8]* %a, i64 0, i64 0
+  call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull %buf)
+  call i8* @strncpy(i8* nonnull %buf, i8* nonnull dereferenceable(1) %src, i64 6)
+  call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull %buf)
+  ret void
+}
+
+define void @dse_strcat(i8* nocapture readonly %src) {
+; OLDPM-LABEL: @dse_strcat(
+; OLDPM-NEXT:    ret void
+;
+; NEWPM-LABEL: @dse_strcat(
+; NEWPM-NEXT:    [[A:%.*]] = alloca [256 x i8], align 16
+; NEWPM-NEXT:    [[BUF:%.*]] = getelementptr inbounds [256 x i8], [256 x i8]* [[A]], i64 0, i64 0
+; NEWPM-NEXT:    call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    [[TMP1:%.*]] = call i8* @strcat(i8* nonnull [[BUF]], i8* nonnull dereferenceable(1) [[SRC:%.*]])
+; NEWPM-NEXT:    call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    ret void
+;
+  %a = alloca [256 x i8], align 16
+  %buf = getelementptr inbounds [256 x i8], [256 x i8]* %a, i64 0, i64 0
+  call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull %buf)
+  call i8* @strcat(i8* nonnull %buf, i8* nonnull dereferenceable(1) %src)
+  call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull %buf)
+  ret void
+}
+
+define void @dse_strncat(i8* nocapture readonly %src) {
+; OLDPM-LABEL: @dse_strncat(
+; OLDPM-NEXT:    ret void
+;
+; NEWPM-LABEL: @dse_strncat(
+; NEWPM-NEXT:    [[A:%.*]] = alloca [256 x i8], align 16
+; NEWPM-NEXT:    [[BUF:%.*]] = getelementptr inbounds [256 x i8], [256 x i8]* [[A]], i64 0, i64 0
+; NEWPM-NEXT:    call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    [[TMP1:%.*]] = call i8* @strncat(i8* nonnull [[BUF]], i8* nonnull dereferenceable(1) [[SRC:%.*]], i64 6)
+; NEWPM-NEXT:    call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull [[BUF]])
+; NEWPM-NEXT:    ret void
+;
+  %a = alloca [256 x i8], align 16
+  %buf = getelementptr inbounds [256 x i8], [256 x i8]* %a, i64 0, i64 0
+  call void @llvm.lifetime.start.p0i8(i64 256, i8* nonnull %buf)
+  call i8* @strncat(i8* nonnull %buf, i8* nonnull dereferenceable(1) %src, i64 6)
+  call void @llvm.lifetime.end.p0i8(i64 256, i8* nonnull %buf)
+  ret void
+}
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+