[PATCH] D145209: [DAE] Don't DAE if we musttail call a "live" (non-DAE-able) function

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:1114
+  }
+}
+
----------------
mtrofin wrote:
> nikic wrote:
> > It's not really clear to me why we need a separate loop to do this, and this can't be included as part of the existing liveness propagation. E.g. surveyFunction() already looks at all the users, and even checks for musttail callers there. Currently, that's only used to mark all the arguments as live, but not the return values. Would it be sufficient to force live return values at that point?
> IIUC, if both argument and return values are marked, it'd be equivalent to the function being marked as live.
> 
> But that aside, we can have A->B->C->x (all are musttail calls), and C has the unanalyzable callsite. That needs to "taint" all in the call chain. We could do that propagation in surveyFunction, but that would be equivalent to this patch, unless I'm missing something?
DAE already propagates liveness across call chains, so it is best to reuse existing propagation is possible. However, after looking a bit closer I don't think we can actually use it, because for return values the propagation will go in the wrong direction. In that case, doing this separate propagation is reasonable.


================
Comment at: llvm/test/Transforms/DeadArgElim/2010-04-30-DbgInfo.ll:111
 !29 = !{}
 !30 = !{i32 1, !"Debug Info Version", i32 3}
----------------
Why the changes to this file? I don't see any musttail usage.


================
Comment at: llvm/test/Transforms/DeadArgElim/musttail-indirect.ll:1
+; RUN: opt -passes=deadargelim,verify -S < %s 2>&1 | FileCheck %s
+define internal i32 @test_caller(ptr %fptr, i32 %a, i32 %b) {
----------------
mtrofin wrote:
> nikic wrote:
> > `verify` is unnecessary, as is `2>&1`, as is the CHECK-NOT. Verification failure causes a crash and will fail the test regardless of output.
> > 
> > Please also update update_test_checks.py.
> done, sending a separate patch w the change in 2010-04-30-DbgInfo.ll, seems unrelated.
This still has an unnecessary `verify` and doesn't use update_test_checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145209