[PATCH] D94106: [Local] Treat calls that may not return as being alive (WIP).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:425
+    // Treat calls that may not return as alive.
+    if (!I->getFunction()->willReturn() && !CB->willReturn())
+      return false;
----------------
fhahn wrote:
> jdoerfert wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > Why does this check for willreturn on the calling function as well? It's not wrong, but it's also not clear to me under what circumstances the extra check would be useful.
> > > What do you think about making this condition `if (!CB->willReturn() && !isa<IntrinsicInst>(CB))`, with a FIXME to drop the intrinsic check later? That avoids the need to block this on intrinsic updates for now, but should be enough to avoid practical miscompiles.
> > I'm not against this but just FTR, most target independent intrinsics have been updated (IIRC). We should ping target owners to do the same for their intrinsics, such as @fhahn did for aarch64.
> That sounds like a good idea. There are just too many intrinsics to update. I started updating X86 but did not have a chance to finish that yet. I'll update the patch.
> Why does this check for willreturn on the calling function as well? It's not wrong, but it's also not clear to me under what circumstances the extra check would be useful.

I think there could be situations where the called function is cannot be `willreturn`, because there are some paths that do not return. But when it is called in this particular function, only the `will return` path will be executed? I guess we could instead also add `will return` at all call-sites (but not the function itself)

I'm not saying this will happen at the moment in practice, but it seems at least conceivable in the future, e.g. if frontends are more aggressively using `willreturn` directly.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:421
+      return false;
+
   if (!I->mayHaveSideEffects())
----------------
jdoerfert wrote:
> Nit: move the comment before the if or use braces.
> 
> `I->getFunction()`
Braces finally added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94106