Page MenuHomePhabricator

[GVN] Verify value equality before doing phi translation for call instruction
ClosedPublic

Authored by wmi on Fri, Aug 30, 10:41 AM.

Details

Summary

This is an updated version of https://reviews.llvm.org/D66909 to fix PR42605.

Basically, current phi translatation translates an old value number to an new value number for a call instruction based on the literal equality of call expression, without verifying there is no clobber in between. This is incorrect.

To get a finegrain check, use MachineDependence analysis to do the job. However, this is still not ideal. Although given a call instruction, MemoryDependenceResults::getCallDependencyFrom returns identical call instructions without clobber in between using MemDepResult with its DepType to be Def. However, identical is too strict here and we want it to be relaxed a little to consider phi-translation -- callee is the same, param operands can be different. That means changing the semantic of MemDepResult::Def and I don't know the potential impact.

So currently the patch is still conservative to only handle MemDepResult::NonFuncLocal, which means the current call has no function local clobber. If there is clobber, even if the clobber doesn't stand in between the current call and the call with the new value, we won't do phi-translate.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Fri, Aug 30, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Aug 30, 10:41 AM
wmi updated this revision to Diff 218134.Fri, Aug 30, 10:42 AM

Add the missing testcase.

asbirlea accepted this revision.Fri, Aug 30, 3:02 PM

Thank you for the patch and offline discussion. LGTM.

This revision is now accepted and ready to land.Fri, Aug 30, 3:02 PM
This revision was automatically updated to reflect the committed changes.
lkail added a subscriber: lkail.Fri, Aug 30, 5:29 PM

Probably good candidate for 9.0.1.

@hans

hans added a comment.Fri, Sep 6, 12:35 AM

Probably good candidate for 9.0.1.

@hans

I've put it on my list. Thanks!