This is an archive of the discontinued LLVM Phabricator instance.

Canonicalize an assume(load != null) into !nonnull metadata
ClosedPublic

Authored by reames on Oct 23 2014, 5:18 PM.

Details

Summary

We currently have two ways of informing the optimizer that the result of a load is never null: metadata and assume. This change converts the second in to the former. This avoids a need to implement optimizations using both forms.

We should probably extend this basic idea to metadata of other forms; in particular, range metadata. We view is that assumes should be considered a "last resort" for when there isn't a more canonical way to represent something.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 15370.Oct 23 2014, 5:18 PM
reames retitled this revision from to Canonicalize an assume(load != null) into !nonnull metadata.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added a reviewer: hfinkel.
reames added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Oct 23 2014, 11:03 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
1113 ↗(On Diff #15370)

It makes me so sad, but this is not correct :(

addr = load
function_that_might_throw()
assume(addr != null)

This has a control dependency on the potentially-throwing function. (Technically, this applies to any function because calling longjmp has the same effect, but LICM only checks mayThrow(), and I see no reason to be more strict anywhere else).

A second, less sad reason why this is not correct is that you need to check the parent of the assume, not the icmp, because they might be in different blocks (and often are after LICM).

In any case, dealing with this "for real" is actually easy: just call

isValidAssumeForContext(II, LHS, DL, DT)
reames updated this revision to Diff 15891.Nov 6 2014, 1:39 PM

Updated version using isAssumeValidForContext.

hfinkel accepted this revision.Nov 11 2014, 12:15 AM
hfinkel edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Nov 11 2014, 12:15 AM
reames closed this revision.Nov 11 2014, 3:24 PM
reames updated this revision to Diff 16065.

Closed by commit rL221737 (authored by @reames).