[PATCH] Stop AliasSetTracker from being overly pessimistic

[Chris Lattner]

On Sep 9, 2013, at 12:16 PM, Krzysztof Parzyszek <kparzysz at codeaurora.org> wrote:

> On 9/9/2013 10:08 AM, Chris Lattner wrote:
>> On Sep 6, 2013, at 5:44 AM, Krzysztof Parzyszek <kparzysz at codeaurora.org> wrote:
>>> This is a follow-up from this thread:
>>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-September/065333.html
>>> 
>>> This patch simply stops volatile (and otherwise "special") loads and stores from pessimizing the entire alias set to which they are added.
>> 
>> Do you understand why that code was there?  Just removing it seems like a bad idea without some analysis.  have you looked at the effect of this on code that uses volatile?  Miscompilations of volatile are not fun to track down.
> 
> I have looked at the users of this code (LICM.cpp, PromoteMemoryToRegister.cpp and BBVectorize.cpp) and I didn't see any cases where removing these lines could lead to problems. Of course I could have missed something, hence I'd like someone to review this.
> 
> LICM checks isSafeToSpeculativelyExecute, which returns "false" for non-unordered loads and all stores. PromoteMemoryToRegister works on local objects (plus it checks for "volatile" directly), and finally, BBVectorize checks for "isSimple".
> 
> I haven't talked directly with the person who wrote those lines, but my guess was that the fact that a volatile load can read a different value every time was meant to be reflected as the "mod" flag on the load's alias set.  However, I haven't seen code that relies on this, and I have a case where this restriction pessimizes customer code.  I believe that the significance of "volatile" is big enough to track it separately, and not rely on AliasSetTracker.

Very likely, I wrote that logic a long time ago (and no, I don't recall the logic for promoting loads to modref).  If clients are already conservative about "isVolatile" sets, then "ok, sounds fine to me".

-Chris