Page MenuHomePhabricator

[BasicAA] Fix handling of invariant group launders
ClosedPublic

Authored by amharc on May 15 2018, 1:10 PM.

Details

Summary

A recent patch (rL331587) to Capture Tracking taught it that the launder_invariant_group intrinsic captures its argument only by returning it. Unfortunately, BasicAA still considered every call instruction as a possible escape source and hence concluded that the result of a launder_invariant_group call cannot alias any local non-escaping value. This led to bug 37458.

This patch updates the relevant check for escape sources in BasicAA.

Diff Detail

Repository
rL LLVM

Event Timeline

amharc created this revision.May 15 2018, 1:10 PM

I went throught it and it looks good to me, I didn't expect that fixing CaptureTracking can lead Alias Analisis to return NoAlias instead of MustAlias. It would be nice if the code would check for such assumptions, but I have no ideas how it could be implemented.

We should also have a direct AA test (something like test/Analysis/BasicAA/cs-cs.ll).

Also, let's not repeat this problem in the future. Please add a comment both here and in CaptureTracking to remind people that intrinsics added to once place must be added to the other place.

llvm/lib/Analysis/BasicAliasAnalysis.cpp
138 ↗(On Diff #146901)

Don't need the { } here.

amharc updated this revision to Diff 147035.May 16 2018, 3:25 AM

Added a direct AA test and cautionary comments in both BasicAA and CaptureTracking, removed unnecessary { }.

amharc marked an inline comment as done.May 16 2018, 3:26 AM

Removed unnecessary { }.

hfinkel accepted this revision.May 16 2018, 4:39 AM

LGTM

This revision is now accepted and ready to land.May 16 2018, 4:39 AM
xbolva00 accepted this revision.May 16 2018, 4:44 AM
Closed by commit rL332466: [BasicAA] Fix handling of invariant group launders (authored by kpszeniczny, committed by ). · Explain WhyMay 16 2018, 6:20 AM
This revision was automatically updated to reflect the committed changes.