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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:89
+bool isCalleeAnalyzable(const CallBase &CB) {
+  return CB.getCalledFunction() && !CB.getCalledFunction()->isDeclaration();
+}
----------------
nikic wrote:
> Is `isDeclaration()` the right criterion here? Shouldn't we be checking for `hasLocalLinkage()`?
Line 544 has the `hasLocalLinkage` bit. The case here is about callees of musttail calls.

Maybe I can rename this to `isMusttailCalleeAnalyzable` and hoist up the rest of the test, for clarity.


================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:1114
+  }
+}
+
----------------
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?


================
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) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145209