This is an archive of the discontinued LLVM Phabricator instance.

[IPSCCP] do not break musttail invariant (PR36485)
ClosedPublic

Authored by indutny on Feb 23 2018, 2:08 PM.

Details

Summary

Do not replace results of musttail calls with a constant if the
call itself can't be removed.

Do not zap returns of musttail callees, if the call site can't be
removed and replaced with a constant.

Do not zap returns of musttail-calling blocks, this breaks
invariant too.

Diff Detail

Event Timeline

indutny created this revision.Feb 23 2018, 2:08 PM
indutny updated this revision to Diff 135713.Feb 23 2018, 3:04 PM

Move test file into a proper directory.

A question: what is backport policy of LLVM project? I'd love to see this fix in 5.0.0 .

indutny updated this revision to Diff 135760.Feb 23 2018, 6:19 PM
indutny removed a reviewer: llvm-commits.
indutny added a subscriber: llvm-commits.

Removed FileCheck from the test. It's enough if it doesn't crash, there's nothing to validate.

I think I've "under-implemented" the fix here. First of all, it doesn't mark callees overdefined, which leads to zapped return value. Second, it basically kills whole optimization because of musttail call site.

What would be the proper implementation in this case? I've few ideas, but I could easily underestimate the scope of the problem.

fhahn added a subscriber: fhahn.

Thanks for the patch! Unless I miss something, we might be able to continue using the more precise lattice value while solving and only handle it when doing the actual replacement. Eli, it would be great if you could have a quick look and see if that makes sense.

Removed FileCheck from the test. It's enough if it doesn't crash, there's nothing to validate.

Could you add some checks again, checking the musttail calls are preserved? Otherwise, we could do lots of things to "not crash".

lib/Transforms/Scalar/SCCP.cpp
1256

This is not the place where the actual replacement happens. I think we could keep the lattice value for the musttail call here and continue using it for solving. The replacement happens in tryToReplaceWithConstant. We could check for musttail calls there and do the replacement there.

This way, in case we can determine the return value of a function containing musttail calls, we still can use the return value at the callsite while solving.

Also, in case the musttail callee has no side effects, the call will be removed (D38856), and we could replace the result with the constant.

indutny updated this revision to Diff 136135.Feb 27 2018, 12:27 PM
indutny marked an inline comment as done.

I've addressed your feedback, and covered more erroneous cases in my patch now.

Thank you!

indutny retitled this revision from [IPSCCP] mark musttail calls overdefined (PR36485) to [IPSCCP] do not break musttail invariant (PR36485).Feb 27 2018, 12:29 PM
indutny edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Feb 28 2018, 12:45 PM

Oh, forgot to ask; do you have commit access? If not, I can commit for you.

fhahn accepted this revision.Feb 28 2018, 1:10 PM

Thanks for updating the patch and thanks Eli for having a look. LGTM too

I don't have commit access yet. Would greatly appreciate if you could land it for me. Thank you!

davide accepted this revision.Feb 28 2018, 4:58 PM

LGTM too. Sorry I'm a little behind with reviews because life took over.

This revision was automatically updated to reflect the committed changes.

Thank you, everyone!