This is an archive of the discontinued LLVM Phabricator instance.

[BasicAA] Fix incorrect scale & offset initializer
ClosedPublic

Authored by njw45 on May 17 2015, 5:40 AM.

Details

Reviewers
hfinkel
Summary

Both scales and offsets in the BasicAliasAnalysis::constantOffsetHeuristic should initially be zero.

Diff Detail

Event Timeline

njw45 updated this revision to Diff 25926.May 17 2015, 5:40 AM
njw45 retitled this revision from to Fix incorrect initializer.
njw45 updated this object.
njw45 edited the test plan for this revision. (Show Details)
njw45 retitled this revision from Fix incorrect initializer to [BasicAA] Fix incorrect scale & offset initializer.May 17 2015, 5:44 AM
njw45 updated this object.
njw45 edited the test plan for this revision. (Show Details)
njw45 added a reviewer: hfinkel.
njw45 added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.May 18 2015, 12:24 AM

It would be much better to have a test case that directly tests the functionality in question, and makes sure the answer is right (instead of just ensuring that it does not crash). Could you extract from your test case a test that you run with -aa-eval? (It seems like this should be relatively easy; if not, please say so.)

njw45 updated this revision to Diff 29052.Jul 5 2015, 1:28 AM
njw45 edited edge metadata.

Un-revert D6682 and make test case re-produce the assertion failure
using just -basicaa -aa-eval

njw45 added a comment.Jul 5 2015, 1:30 AM

Could you extract from your test case a test that you run with -aa-eval?

Sorry for the delay - I've modified test/Analysis/BasicAA/bug.23540.ll so it hits the assertion error just using -aa-eval. I've also added in the rest of D6682 as it was (correctly) reverted in r237984. Thanks -

Nick

hfinkel accepted this revision.Jul 8 2015, 9:55 PM
hfinkel edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jul 8 2015, 9:55 PM
njw45 added a comment.Jul 9 2015, 8:56 AM

@hfinkel thanks - but I still don't have commit permissions, so could you commit this for me?

hfinkel closed this revision.Jul 11 2015, 4:05 AM

r241981, thanks!