This is an archive of the discontinued LLVM Phabricator instance.

[CFLAA] Revert all modref supports due to their buggy nature
ClosedPublic

Authored by grievejia on Jul 29 2016, 2:51 PM.

Details

Summary

Unfortunately, the getModRefBehavior() functions for both CFL-AA turn out to be very buggy.

Currently, getModRefBehavior() is implemented by looking at AliasSummary.RetParamRelations, a vector that records how pointer values are accessed. Non-pointer values are simply ignored. As a result, if inside the function we store an i32 to one of the i32* parameter, or if we load an i64 from one of the i64* parameter, the load/store won't be recorded by RetParamRelations since i32 and i64 are scalar types, not pointer types. If we only check RetParamRelations for modref behavior, we will inevitably miss those scalar loads and stores. The takeaway point here is that RetParamRelations is good for summarizing pointer aliases, but cannot be relied on to summarize function modref behaviors.

Another problem with our current getModRefBehavior() implementation is that when analyzing modref for function f, it does not consider any functions that are called by f. For example, if f() is a function that calls certain library function with side-effects, those side effects won't be aggregated into f's modref behavior, which is problematic.

The lesson I learned from debugging those problems is that modref and aliasing are conceptually two different problems. Aliasing can be used to derive modrefs, but they're still two separate analyses and conflating the two into a single pass doesn't sound like a good idea. I'd say that let's remove the buggy implementation for now until we can come up with a more principled, less error-prone approach .

Diff Detail

Event Timeline

grievejia updated this revision to Diff 66174.Jul 29 2016, 2:51 PM
grievejia retitled this revision from to [CFLAA] Revert all modref supports due to their buggy nature.
grievejia updated this object.
grievejia added a subscriber: llvm-commits.
george.burgess.iv edited edge metadata.

LGTM, thanks for the patch!

This revision is now accepted and ready to land.Aug 1 2016, 11:44 AM
This revision was automatically updated to reflect the committed changes.