thr3ads.net - llvm dev - [llvm-dev] Hoisting in the presence of volatile loads. [Dec 2017]

If this information is useful, please help other people find it:
Share via:

Hal Finkel via llvm-dev

2017-Dec-21 05:17 UTC

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

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.

  -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/20171220/e3a0552c/attachment-0001.html>

Alina Sbirlea via llvm-dev

2017-Dec-21 23:11 UTC

head link

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

Thank you for the lang ref pointer, Hal!
I sent out the change in D41525 <https://reviews.llvm.org/D41525>.

Best,
Alina

On Wed, Dec 20, 2017 at 9:17 PM, Hal Finkel <hfinkel at anl.gov> 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.
>
>  -Hal
>
>
> Thoughts?
>
>
> Thanks,
> Alina
>
>
> On Wed, Dec 20, 2017 at 11:50 AM, Krzysztof Parzyszek <
> 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 listllvm-dev at
lists.llvm.orghttp://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/20171221/b1d17fef/attachment.html>

Philip Reames via llvm-dev

2017-Dec-22 23:53 UTC

head link

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

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>

Alina Sbirlea via llvm-dev

2017-Dec-23 04:42 UTC

head link

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

On Dec 22, 2017 15:53, "Philip Reames" <listmail at
philipreames.com> wrote:

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.

Thank you for the reply and confirming the approach!

Best,
Alina

 -Hal

Thoughts?

Thanks,
Alina

On Wed, Dec 20, 2017 at 11:50 AM, Krzysztof Parzyszek <
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
listllvm-dev at
lists.llvm.orghttp://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/5e136346/attachment.html>

Ralf Jung via llvm-dev

2017-Dec-23 13:42 UTC

head link

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

Hi all,
>> 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.
Please forgive my possible naive question, but I do not understand how
"accessing a volatile object through a non-volatile pointer is UB" is
enough to
justify this optimization.  Don't you also need "accessing a
non-volatile object
through a volatile pointer is UB"?

In other words, if I understand correctly, this proposed reordering could change
the behavior of the following program:  <https://godbolt.org/g/zH1Tey>


static void foo(int *x, volatile int *y) {
    // These may be reordered now?
    *x = 4;
    *y = 5;
}

int bar() {
    int x = 0;
    foo(&x, (volatile int*)&x);
    return x; // MUST return 5
}


`x` and `y` alias in `foo` and both point to a non-volatile object; reordering
them changes what `bar` returns.  Notice that casts to volatile pointers are
pretty common e.g. in the Linux kernel:

  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))

Am I missing something?

Kind regards,
Ralf

Reasonably Related Threads

Search for more reasonably related threads

llvm dev - Dec 2017 - Hoisting in the presence of volatile loads.

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

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

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

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

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

Reasonably Related Threads