[llvm-dev] Hoisting in the presence of volatile loads.

[Philip Reames via llvm-dev]

On 12/20/2017 09:17 PM, Hal Finkel wrote:
>
>
> On 12/20/2017 03:49 PM, Alina Sbirlea via llvm-dev wrote:
>> +Philip to get his input too.
>> I've talked with George offline, and here's a summary:
>>
>> In D16875 <https://reviews.llvm.org/D16875>, the decision made was: 
>> "The LLVM spec is ambiguous about whether we can hoist a non-volatile 
>> load above a volatile load when the loads alias. It's probably best 
>> not to exploit this ambiguity at the moment by unconditionally 
>> allowing the motion of nonvolatile loads above volatile loads (and 
>> vice versa)"
>> So the testcase: test/Analysis/MemorySSA/volatile-clobber.ll, is 
>> checking that a volatile load is the defining access of a load with 
>> which it may alias.
>>
>> Snippet:
>> ; Ensuring that we don't automatically hoist nonvolatile loads around 
>> volatile
>> ; loads
>> ; CHECK-LABEL define void @volatile_only
>> define void @volatile_only(i32* %arg1, i32* %arg2) {
>> [...]
>> ; MayAlias
>> ; CHECK: 2 = MemoryDef(1)
>> ; CHECK-NEXT: load volatile i32, i32* %arg1
>>   load volatile i32, i32* %arg1
>> ; CHECK: MemoryUse(2)
>> ; CHECK-NEXT: load i32, i32* %arg2
>>   load i32, i32* %arg2
>>   ret void
>> }
>>
>> The testcase: test/Transforms/LICM/volatile-alias.ll checks the 
>> opposite, that we *do* hoist the non-volatile load. This is currently 
>> ensured by the AliasSetTracker.
>>
>>
>> The conclusion I'm drawing is that right now AliasSetTracker and 
>> MemorySSA have different behaviors and replacing one with the either 
>> will naturally lead to different outcomes.
>> So, how can I make progress here?
>>
>>
>> I think it's reasonable in D40375 
>> <https://reviews.llvm.org/D40375> to 
>> have pointerInvalidatedByLoopWithMSSA only check if the defining 
>> access is within the current loop or liveOnEntry, and rely on 
>> MemorySSA to either consider a volatile load a clobbering access or 
>> not. So, right now the LICM/volatile-alias.ll testcase will behave 
>> differently with MemorySSA enabled.
>>
>> A separate decision, is whether to update getLoadReorderability in 
>> MemorySSA to remove the restriction that loads should not alias. That 
>> would give the same behavior as the AliasSetTracker.
>> George's recollection is that the reason MemorySSA didn't reorder 
>> aggressively is because it was untested at the time. Now that 
>> MemorySSA is used more widely, it may be a good time to revisit the 
>> initial decision?
>
> I think that this is the right way to approach this: we should change 
> MemorySSA to be less conservative in this regard. LLVM's language 
> reference is pretty explicit about reordering volatile and 
> non-volatile operations:
>
>> The optimizers must not change the number of volatile operations or 
>> change their order of execution relative to other volatile 
>> operations. The optimizers/may/change the order of volatile 
>> operations relative to non-volatile operations. This is not Java’s 
>> “volatile” and has no cross-thread synchronization behavior.
>
> I see no reason to prevent this kind of reordering, even if the 
> volatile and non-volatile accesses might alias.
Just to chime in here since I was explicitly CCd, I agree with this 
conclusion and believe this thread reached the proper result per the 
appropriate specs.
>
>  -Hal
>
>>
>> Thoughts?
>>
>>
>> Thanks,
>> Alina
>>
>>
>> On Wed, Dec 20, 2017 at 11:50 AM, Krzysztof Parzyszek 
>> <kparzysz at codeaurora.org <mailto:kparzysz at codeaurora.org>> wrote:
>>
>>     On 12/20/2017 1:37 PM, Sanjoy Das wrote:>
>>
>>         Fwiw, I was under the impression that regular loads could
>>         *not* be
>>         reordered with volatile loads since we could have e.g.:
>>
>>            int *normal = &global_variable;
>>            volatile int* ptr = 0;
>>            int k = *ptr; // segfaults, and the signal handler writes
>>         to *normal
>>            int value = *normal;
>>
>>         and that we'd have to treat volatile loads and stores
>>         essentially as
>>         calls to unknown functions.
>>
>>
>>     For this to work, "normal" should be volatile as well.
>>
>>     -Krzysztof
>>
>>
>>     -- 
>>     Qualcomm Innovation Center, Inc. is a member of Code Aurora
>>     Forum, hosted by The Linux Foundation
>>
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> -- 
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20171222/ec6f45a8/attachment.html>