Page MenuHomePhabricator

Create PHI node for the return value only when the return value has uses.
ClosedPublic

Authored by twoh on Aug 25 2017, 9:12 PM.

Details

Summary

Currently, a phi node is created in the normal destination to unify the return values from promoted calls and the original indirect call. This patch makes this phi node to be created only when the return value has uses.

This patch is necessary to generate valid code, as compiler crashes with the attached test case without this patch. Without this patch, an illegal phi node that has no incoming value from entry/catch is created in cleanup block.

I think existing implementation is good as far as there is at least one use of the original indirect call. insertCallRetPHI creates a new phi node in the normal destination block only when the original indirect call dominates its use and the normal destination block. Otherwise, fixupPHINodeForNormalDest will handle the unification of return values naturally without creating a new phi node. However, if there's no use, insertCallRetPHI still creates a new phi node even when the original indirect call does not dominate the normal destination block, because getCallRetPHINode returns false.

Diff Detail

Repository
rL LLVM

Event Timeline

twoh created this revision.Aug 25 2017, 9:12 PM
twoh edited the summary of this revision. (Show Details)Aug 25 2017, 10:09 PM
davidxl added inline comments.Aug 28 2017, 8:49 AM
lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
592 ↗(On Diff #112781)

may be wrap this check inside the function just as other checks?

twoh updated this revision to Diff 112903.Aug 28 2017, 8:50 AM

Fix a test.

xur edited edge metadata.Aug 28 2017, 10:24 AM

Thanks for the fix.

LGTM, but I agree with David that it's preferable to put the check inside insertCallRetPHI() (around isVoidTy()).

twoh updated this revision to Diff 112917.Aug 28 2017, 10:43 AM

Addressing @davidxl and @xur's comment. Thanks!

xur accepted this revision.Aug 28 2017, 10:44 AM

LGTM

This revision is now accepted and ready to land.Aug 28 2017, 10:44 AM
This revision was automatically updated to reflect the committed changes.