[DeadArgElim] Set unused arguments for internal functions

[qcolombet]

Summary

Prior to this patch we would only set to undef the unused arguments of the
external functions. The rationale was that unused arguments of internal
functions wouldn't need to be turned into undef arguments because they
should have been simply eliminated by the time we reach that code.

This is actually not true because there are plenty of cases where we can't
remove unused arguments. For instance, if the internal function is used in
an indirect call, it may not be possible to change the function signature.
Yet, for statically known call-sites we would still like to mark the unused
arguments as undef.

This patch enables the "set undef arguments" optimization on internal
functions when we encounter cases where internal functions cannot be
optimized. I.e., whenever an internal function is marked "live".

[fhahn]

LGTM, thanks!

[dyung]

Hi @qcolombet, your change seems to be causing 2 test failures on the PS4 build bots, can you please take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/21146
https://lab.llvm.org/buildbot/#/builders/216/builds/3762

[thakis]

I think this broke check-clang everywhere, e.g.:

https://lab.llvm.org/buildbot/#/builders/109/builds/37754
http://45.33.8.238/linux/75141/step_7.txt
http://45.33.8.238/win/57380/step_7.txt
http://45.33.8.238/macm1/34477/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

[qcolombet]

Ah thanks for the heads-up, looking at the clang issue.

[qcolombet]

Alright, at first I thought it was a latent bug in the dead argument elimination pass and now I think the two failing tests just need updating.
I'll upload a diff with the fix tests. (See below for the diff of the tests themselves.)

Details

I initially thought that we were wrongly optimizing noundef arguments. However, I believe that's not the case, because when we replace an argument with undef, we know that there is no use of this argument, so no undefined behavior is attached to it. This was clearly taken into account because the pass properly removes all the UB implying attributes (see Fn.removeParamAttrs(Arg.getArgNo(), UBImplyingAttributes); later in the method I modified.)

Now, regarding the two clang tests that failed: One is expecting the debug info to contain a reference to an argument that is not used and that the optimized replaced by undef and the other is tripping on the fact that we can use fastcc since we proved that the address of the block function is not used.

Here the full diff for fixing the tests.

diff --git a/clang/test/CodeGen/debug-info-block-vars.c b/clang/test/CodeGen/debug-info-block-vars.c
index dc522a807951..11c899fa3c81 100644
--- a/clang/test/CodeGen/debug-info-block-vars.c
+++ b/clang/test/CodeGen/debug-info-block-vars.c
@@ -11,7 +11,10 @@
 // CHECK: call void @llvm.dbg.declare(metadata i8** %.block_descriptor.addr,
 // CHECK-SAME:                        metadata !DIExpression())
 // CHECK-OPT-NOT: alloca
-// CHECK-OPT: call void @llvm.dbg.value(metadata i8* %.block_descriptor,
+// Since the block address is not used anywhere in this function,
+// the optimizer (DeadArgElim) has replaced all the false uses
+// (i.e., metadata users) with undef.
+// CHECK-OPT: call void @llvm.dbg.value(metadata i8* undef,
 // CHECK-OPT-SAME:                      metadata !DIExpression())
 void f(void) {
   a(^{
diff --git a/clang/test/CodeGenObjCXX/nrvo.mm b/clang/test/CodeGenObjCXX/nrvo.mm
index 89d9ae9639cc..0e4b98996965 100644
--- a/clang/test/CodeGenObjCXX/nrvo.mm
+++ b/clang/test/CodeGenObjCXX/nrvo.mm
@@ -22,7 +22,11 @@ - (X)getNRVO {
 
 X blocksNRVO() {
   return ^{
-    // CHECK-LABEL: define internal void @___Z10blocksNRVOv_block_invoke
+    // With the optimizer enabled, the DeadArgElim pass is able to
+    // mark the block litteral address argument as unused and later the
+    // related block_litteral global variable is removed.
+    // This allows to promote this call to a fastcc call.
+    // CHECK-LABEL: define internal fastcc void @___Z10blocksNRVOv_block_invoke
     X x;
     // CHECK: call void @_ZN1XC1Ev
     // CHECK-NEXT: ret void

[qcolombet]

• includes fixes to clang tests that were missed in the original commit.

If someone knows who we can add from the clang side to double check the tests update that would be great!

[qcolombet]

Hi @thakis , @dyung , @vitalybuka,

Thanks for the heads-up and the revert.

Fixed clang tests included in the diff.

Cheers,
-Quentin

[thakis]

Updating the tests makes sense to me, fwiw.

[qcolombet]

Thanks @thakis !

@fhahn are you okay with the clang tests update as well?

[fhahn]

In D124699#3507250, @qcolombet wrote:

Thanks @thakis !

@fhahn are you okay with the clang tests update as well?

Yes looks good, thanks!

[kstoimenov]

This change is breaking memory sanitizer in some cases. We observed that when the argument is not actually removed the pass is dropping the noundef attribute. See the diff snippet below. Is that an intended behavior?

We would like to revert this as it is breaking MSan tests.

Thanks,
Kirill

< define internal %struct._object* @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object* noundef %args) #0 !dbg !1065 {
---
> define internal %struct._object* @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object* %args) #0 !dbg !1065 {

[vitalybuka]

In D124699#3549191, @kstoimenov wrote:

This change is breaking memory sanitizer in some cases. We observed that when the argument is not actually removed the pass is dropping the noundef attribute. See the diff snippet below. Is that an intended behavior?

We would like to revert this as it is breaking MSan tests.

Thanks,
Kirill

< define internal %struct._object* @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object* noundef %args) #0 !dbg !1065 {
---
> define internal %struct._object* @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object* %args) #0 !dbg !1065 {

I guess I realized why msan, is broken. It's indirectly cased by this patch, but the problem is in our code.
This patch poisons more msan arguments (see NOTE below), but still correctly.
However I just noticed that the issues we are working with Kirill involves a peace of notinstrumented assembly, which fails to propagate Msan shadow. @kstoimenov and me will find a different solution.

*NOTE* This optimization is negative for msan with noundef mode (-fsanitizer-memory-param-retval)
If we remove noundef, then msan must to setup TLS for arguments. If it's undef, we set that into -1 to poison arg.
For msan it could be more efficient to keep noundef and pass null into that argument.
But that's not a problem of this patch, and we will address that later if needed.

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

268

@kstoimenov Offline I guessed that maybe our problem because we lost noundef on weak function arg, however this check protects against that.

[qcolombet]

But that's not a problem of this patch, and we will address that later if needed.

Thanks for looking into it.

clang/test/CodeGen/debug-info-block-vars.c

 // RUN: %clang_cc1 -no-opaque-pointers -x c -std=c89 -fblocks -debug-info-kind=standalone -emit-llvm -O0 \
 // RUN:   -triple x86_64-apple-darwin -o - %s | FileCheck %s
 // RUN: %clang_cc1 -no-opaque-pointers -x c -std=c89 -fblocks -debug-info-kind=standalone -emit-llvm -O1 \
 // RUN:   -triple x86_64-apple-darwin -o - %s \
 // RUN:   | FileCheck --check-prefix=CHECK-OPT %s
 
 // CHECK: define internal void @__f_block_invoke(i8* noundef %.block_descriptor)
 // CHECK: %.block_descriptor.addr = alloca i8*, align 8
 // CHECK: %block.addr = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor* }>*, align 8
 // CHECK: store i8* %.block_descriptor, i8** %.block_descriptor.addr, align 8
 // CHECK: call void @llvm.dbg.declare(metadata i8** %.block_descriptor.addr,
 // CHECK-SAME:                        metadata !DIExpression())
 // CHECK-OPT-NOT: alloca
-// CHECK-OPT: call void @llvm.dbg.value(metadata i8* %.block_descriptor,
+// Since the block address is not used anywhere in this function,
+// the optimizer (DeadArgElim) has replaced all the false uses
+// (i.e., metadata users) with undef.
+// CHECK-OPT: call void @llvm.dbg.value(metadata i8* undef,
 // CHECK-OPT-SAME:                      metadata !DIExpression())
 void f(void) {
   a(^{
     b();
   });
 }

clang/test/CodeGenObjCXX/nrvo.mm

- (X)getNRVO {
   // CHECK: call void @_ZN1XC1Ev
   // CHECK-NEXT: ret void
   return x;
 }
 @end
 
 X blocksNRVO() {
   return ^{
-    // CHECK-LABEL: define internal void @___Z10blocksNRVOv_block_invoke
+    // With the optimizer enabled, the DeadArgElim pass is able to
+    // mark the block litteral address argument as unused and later the
+    // related block_litteral global variable is removed.
+    // This allows to promote this call to a fastcc call.
+    // CHECK-LABEL: define internal fastcc void @___Z10blocksNRVOv_block_invoke
     X x;
     // CHECK: call void @_ZN1XC1Ev
     // CHECK-NEXT: ret void
     return x;
   }() ;
 }
 

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

bool DeadArgumentEliminationPass::RemoveDeadArgumentsFromCallers(Function &Fn) {
   // semantics. In the below example, the dead load from %p may not have been
   // eliminated from the linker-chosen copy of f, so replacing %p with undef
   // in callers may introduce undefined behavior.
   //
   // define linkonce_odr void @f(i32* %p) {
   //   %v = load i32 %p
   //   ret void
   // }
   if (!Fn.hasExactDefinition())
     return false;
 
-  // Functions with local linkage should already have been handled, except the
-  // fragile (variadic) ones which we can improve here.
-  if (Fn.hasLocalLinkage() && !Fn.getFunctionType()->isVarArg())
+  // Functions with local linkage should already have been handled, except if
+  // they are fully alive (e.g., called indirectly) and except for the fragile
+  // (variadic) ones. In these cases, we may still be able to improve their
+  // statically known call sites.
+  if ((Fn.hasLocalLinkage() && !LiveFunctions.count(&Fn)) &&
+      !Fn.getFunctionType()->isVarArg())
     return false;
 
   // Don't touch naked functions. The assembly might be using an argument, or
   // otherwise rely on the frame layout in a way that this analysis will not
   // see.
   if (Fn.hasFnAttribute(Attribute::Naked))
     return false;
 

llvm/test/Transforms/DeadArgElim/fct_ptr.ll

+; RUN: opt -S %s -deadargelim -o - | FileCheck %s
+; In that test @internal_fct is used by an instruction
+; we don't know how to rewrite (the comparison that produces
+; %cmp1).
+; Because of that use, we used to bail out on removing the
+; unused arguments for this function.
+; Yet, we should still be able to rewrite the direct calls that are
+; statically known, by replacing the related arguments with undef.
+; This is what we check on the call that produces %res2.
+
+define i32 @call_indirect(i32 (i32, i32, i32)* readnone %fct_ptr, i32 %arg1, i32 %arg2, i32 %arg3) {
+; CHECK-LABEL: @call_indirect(
+; CHECK-NEXT:    [[CMP0:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR:%.*]], @external_fct
+; CHECK-NEXT:    br i1 [[CMP0]], label [[CALL_EXT:%.*]], label [[CHK2:%.*]]
+; CHECK:       call_ext:
+; CHECK-NEXT:    [[RES1:%.*]] = tail call i32 @external_fct(i32 undef, i32 [[ARG2:%.*]], i32 undef)
+; CHECK-NEXT:    br label [[END:%.*]]
+; CHECK:       chk2:
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR]], @internal_fct
+; CHECK-NEXT:    br i1 [[CMP1]], label [[CALL_INT:%.*]], label [[CALL_OTHER:%.*]]
+; CHECK:       call_int:
+; CHECK-NEXT:    [[RES2:%.*]] = tail call i32 @internal_fct(i32 undef, i32 [[ARG2]], i32 undef)
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       call_other:
+; CHECK-NEXT:    [[RES3:%.*]] = tail call i32 @other_fct(i32 [[ARG2]])
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    [[FINAL_RES:%.*]] = phi i32 [ [[RES1]], [[CALL_EXT]] ], [ [[RES2]], [[CALL_INT]] ], [ [[RES3]], [[CALL_OTHER]] ]
+; CHECK-NEXT:    ret i32 [[FINAL_RES]]
+;
+  %cmp0 = icmp eq i32 (i32, i32, i32)* %fct_ptr, @external_fct
+  br i1 %cmp0, label %call_ext, label %chk2
+
+call_ext:
+  %res1 = tail call i32 @external_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+  br label %end
+
+chk2:
+  %cmp1 = icmp eq i32 (i32, i32, i32)* %fct_ptr, @internal_fct
+  br i1 %cmp1, label %call_int, label %call_other
+
+call_int:
+  %res2 = tail call i32 @internal_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+  br label %end
+
+call_other:
+  %res3 = tail call i32 @other_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+  br label %end
+
+end:
+  %final_res = phi i32 [%res1, %call_ext], [%res2, %call_int], [%res3, %call_other]
+  ret i32 %final_res
+}
+
+
+define i32 @external_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+  ret i32 %arg2
+}
+
+define internal i32 @internal_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+  ret i32 %arg2
+}
+
+define internal i32 @other_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+  ret i32 %arg2
+}
+