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

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Jan 23 07:54:02 PST 2021


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;
----------------
nikic wrote:
> nikic wrote:
> > fhahn wrote:
> > > 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.
> > > > 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.
> > 
> > Right, I'd say that in this case the right thing to do is annotate the call-site. I believe that the Attributor also does inference in that direction (while FuncAttrs does not). If this is not needed to prevent a regression right now, then I would drop the check, as it's logically not the right place to do it. It should be handled by attribute inference.
> To add one more note to this: This kind of reasoning is applicable to most attributes. For example, we could replace the `!I->mayHaveSideEffects()` call below with something to the effect of `!I->mayHaveSideEffects() && !I->getFunction()->mayHaveSideEffects()`, and generally do this for many other call-site attribute checks. But I don't think we want to do that generally, and for that reason also don't think we should do that here.
I mostly added it out of caution, but there are no binary changes without it (on MultiSource  &SPEC). I'll drop it, let's see if there are any regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94106



More information about the llvm-commits mailing list