[PATCH] D7864: This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h

George Burgess IV via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 1 17:36:02 PST 2016

george.burgess.iv marked 10 inline comments as done.
george.burgess.iv added a comment.

After further investigation, the rest of the comments seem to be dead/already addressed, too; marked them as done.

Also, wow. Responding to comments at the bottom *really* didn't go well. I thought it would reflow them, but apparently not. Sorry about that. :/

Either way, I'll leave this open for a bit, in case anyone has any other remarks. Assuming Danny's LGTM is sufficient to let me commit, I'll do so lateish Wednesday if there are no objections by then. :)

Trying to make the comment/response chain at the bottom look less confusing...


> Er, what? This just seems flat out wrong. Two loads on the same thread to the same location are always ordered regardless of flags. Ordering doesn't bypass a mustalias...

>  Unless this is absolutely required, I would request you separate this and review it separately.

Nuked `possiblyAffectedBy` from orbit; it's an optimization that can be readded later.

> Does this optimization actually help? Just curious.

Marked with a TODO so we can look into that when we have users of MemorySSA/a more complete API/...

> This seems suspicious to me as said above.

>  More generally, the smarts of this routine deserve to be reviewed independently.

`possiblyAffectedBy` was removed, so I think this comment doesn't apply anymore (nor does my initial response to it). :)


> You might also want to check AA->pointsToConstantMemory here.

I think it's better to get MemorySSA in and work on little optimizations like this after that. Removed the invariant check (but kept the test as an XFAIL), to be readded in the near future as a set of enhancements. (Good point, though!)

> This algorithm in getClobberingMemoryAccess does not seem correct [...]

Seems old; we now use DFS. Marking as done, but added a test case of your second example.


More information about the llvm-commits mailing list