[PATCH] Stop AliasSetTracker from being overly pessimistic

[Krzysztof Parzyszek]

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.

-K

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation