[PATCH] D46282: [AA] cfl-anders-aa with field sensitivity

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv reopened this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

rja - I'm very sorry if this comes off as rude, but do you have some sort of familiarity with CFLAA/are you a new account for someone who's worked in this area before? If not, it's really not OK to solo-LGTM changes like this. If you don't intend to approve a whole patch, please be very explicit about it in the future, with comments like "this LGTM, but please wait for someone else's approval."

I've reverted this patch for the moment (r332674), since I see no indication that rja is familiar enough with this code to LGTM a patch of this nature.

I'm sorry that it's taking me a while to get to this patch, but I'll try to quickly summarize my concerns. Please note that I haven't read the patch, and am purely doing this in response to the LGTM:

- I'm concerned about how unification works here. e.g. If I do a single write to two pointer fields (e.g. writing at an offset of two bytes in `struct Foo { void *p, *v; };`, assuming 32-bit ptrs), how does that affect the aliasing between `p` and `v` (and the value that was written)? Types in LLVM aren't trustworthy, and can be bitcasted between freely, so relying on e.g. GEP field offsets to represent fields is shaky at best.
- I'm concerned about what happens in the face of unknown offsets (e.g. offsets into arrays, ...).
- I'm concerned about heaping more features onto an AA that's known to be broken in a few ways. AA is notoriously difficult (IME) to debug; making it more powerful without fixing those just sounds like a recipe for trouble to me.
- I'm concerned about the testing story for this. I'd usually say "if it bootstraps clang, that's sufficient," but I don't think that CFLAnders did that cleanly the last time we tried. Maybe CFLSteens (which I would've expected to see updated, as well) would be sufficient, though?

Again, I do appreciate this patch and the review help. I'm just thrown off by that this was accepted by someone who, AFAICT, doesn't know this code.


Repository:
  rL LLVM

https://reviews.llvm.org/D46282