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

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

+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?

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171220/898bf4c1/attachment.html>

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>

Hello.
This might be a trivial question, but is it correct if the signal handler
aborts the program?
For example:

  int *normal = ADDRESS1;
  volatile int* ptr = ADDRESS2;
  int k = *ptr; // segfaults, and the signal handler calls abort()
  int value = *normal; // this may be UB(undefined behavior).

Let's assume that normal is dereferenceable if ptr1 does not raise SEGSEGV,
but accessing normal raises UB if ptr1 raises SEGSEGV.
Before reordering is done, there's no UB because the program will always
abort if UB could happen, but UB may happen after they are reordered.

Best Regards,
Juneyoung Lee

On Thu, Dec 21, 2017 at 4:50 AM, Krzysztof Parzyszek via llvm-dev <
llvm-dev at lists.llvm.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
>


-- 

Juneyoung Lee
Software Foundation Lab, Seoul National University
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171224/6acac74b/attachment.html>

On 12/23/2017 01:56 PM, Juneyoung Lee via llvm-dev
wrote:
> Hello.
> This might be a trivial question, but is it correct if the signal 
> handler aborts the program?
Yes, because the actions of SEGSEGV signal handlers don't fall within 
the abstract machine we're modeling. If they did, we'd need to treat all
volatile memory accesses as arbitrary function calls (because that's 
exactly what they'd be). This would inhibit our ability to do almost any 
kind of pointer-aliasing reasoning about the volatile accesses (i.e., 
they'd alias with anything that might be visible to an external 
function), and moreover, they'd all be read/write (because that external 
function might do anything).

Another way to look at this is: Once the SEGSEGV is generated, the 
program has already triggered UB (regardless of whether the access is 
volatile and regardless of what the handler might do). This UB is just 
as undefined as any other.

When we've discussed this in the past, I believe the recommendation was 
that, if we want to support a mode in which volatile accesses have these 
kinds of semantics (and there are certainly environments in which this 
might make sense), then we should have Clang lower them into some kind 
of "volatile-access intrinsics" because it will be easy to make these 
look like arbitrary read/write function calls in the optimizer.

  -Hal
> For example:
>
>   int *normal = ADDRESS1;
>   volatile int* ptr = ADDRESS2;
>   int k = *ptr; // segfaults, and the signal handler calls abort()
>   int value = *normal; // this may be UB(undefined behavior).
>
> Let's assume that normal is dereferenceable if ptr1 does not raise 
> SEGSEGV, but accessing normal raises UB if ptr1 raises SEGSEGV.
> Before reordering is done, there's no UB because the program will 
> always abort if UB could happen, but UB may happen after they are 
> reordered.
>
> Best Regards,
> Juneyoung Lee
>
> On Thu, Dec 21, 2017 at 4:50 AM, Krzysztof Parzyszek via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.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 <mailto:llvm-dev at lists.llvm.org>
>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>     <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>
>
>
>
> -- 
>
> Juneyoung Lee
> Software Foundation Lab, Seoul National University
>
>
> _______________________________________________
> 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/20180107/d172501a/attachment.html>