Page MenuHomePhabricator

[LangRef] Clarify semantics of volatile operations.
ClosedPublic

Authored by efriedma on Oct 11 2018, 7:43 PM.

Details

Summary

Specifically, clarify the following:

  1. Volatile load and store may access addresses that are not memory (MMIO registers, etc.).
  2. Volatile load and store do not modify unrelated memory.
  3. Volatile load and store do not trap.

Prompted by recent discussion of volatile on llvmdev. I think this is enough to resolve all the issues brought up in that discussion, at least from the perspective of LangRef.

Currently, there's sort of a split in the LLVM source code about whether volatile operations are allowed to trap; this resolves that dispute in favor of not allowing them to trap. (This is the opposite of the position I originally took, but thinking about it a bit more it makes sense for the uses we expect.)

Hal brought up the possibility on llvmdev that certain APIs like isSafeToLoadUnconditionally and FindAvailablePtrLoadStore might assume that volatile operations do in fact point to memory; I think we should just treat that as a bug, though.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Oct 11 2018, 7:43 PM

Currently, there's sort of a split in the LLVM source code about whether volatile operations are allowed to trap; this resolves that dispute in favor of not allowing them to trap. (This is the opposite of the position I originally took, but thinking about it a bit more it makes sense for the uses we expect.)

I agree. They shouldn't trap.

Hal brought up the possibility on llvmdev that certain APIs like isSafeToLoadUnconditionally and FindAvailablePtrLoadStore might assume that volatile operations do in fact point to memory; I think we should just treat that as a bug, though.

I agree. We should not use the presence of a volatile operation to justify adding other non-volatile accesses to the same address (and, by definition, we can't add other volatile accesses).

docs/LangRef.rst
2188 ↗(On Diff #169350)

How should we specify the interaction between this and inaccessiblememonly?

> Hal brought up the possibility on llvmdev that certain APIs like isSafeToLoadUnconditionally and FindAvailablePtrLoadStore might assume that volatile operations do in fact point to memory; I think we should just treat that as a bug, though.
I agree. We should not use the presence of a volatile operation to justify adding other non-volatile accesses to the same address (and, by definition, we can't add other volatile accesses).

Can we then also expand this part of the reference to clarify that optimizers must not change the number of volatile *or* nonvolatile operations to memory which is only accessed via volatile operations (with a note one way or the other WRT whether this includes loads/stores from pointers marked "dereferenceable")?

Certain memory accesses, such as load’s, store’s, and llvm.memcpy’s may be marked volatile. 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.

What thread was that discussion in? Accesses to device-mapped memory can trap, what is the reason for the compiler to assume that they can't?

llvmdev thread is titled "Volatile and Inserted Loads/Stores on MMIO​".

Accesses to device-mapped memory can trap, what is the reason for the compiler to assume that they can't?

If we don't restrict the effects of a volatile load/store, we have to assume it could, at worst, modify any memory that isn't local to the function and unwind the stack, like a call to an unknown function. That would block optimizations around volatile accesses. That's not what LLVM implements, and it's not what other compilers implement.

Can we then also expand this part of the reference to clarify that optimizers must not change the number of volatile *or* nonvolatile operations to memory which is only accessed via volatile operations

This is implied by "a volatile load or store may access addresses which do not point to memory", but sure, I can expand that to state it explicitly.

efriedma added inline comments.Oct 16 2018, 2:39 PM
docs/LangRef.rst
2188 ↗(On Diff #169350)

This is essentially supposed to be inaccessiblememonly (plus the pointed-to address, if it points into memory).

I'll take another shot at the wording.

efriedma updated this revision to Diff 169900.Oct 16 2018, 3:19 PM

Took another shot at the wording. This should be a bit more explicit about what we're assuming, and what transforms are allowed.

This revision is now accepted and ready to land.Nov 12 2018, 8:43 PM

LGTM

I still think it would be better to explicitly say something about the access properties here relative to inaccessiblememonly functions. Also, are we saying that volatile loads can write to this inaccessible memory?

I can add a sentence to specifically state that this can modify state accessed by functions marked with the inaccessiblememonly attribute, and vice versa, if that's the interaction you're worried about. (I don't think it's practical to have multiple forms of inaccessible state.)

I can add a sentence to specifically state that this can modify state accessed by functions marked with the inaccessiblememonly attribute, and vice versa, if that's the interaction you're worried about. (I don't think it's practical to have multiple forms of inaccessible state.)

Thanks. Regarding this:

Also, are we saying that volatile loads can write to this inaccessible memory?

can you please clarify your thoughts?

Yes, volatile loads and stores access the "same" state as inaccessiblememonly functions, I think.

In general, we have to make sure volatile operations don't get reordered. This is equivalent to saying that volatile operations read/modify some inaccessible state (something like a list of volatile accesses in the program). This essentially the same as the language for inaccessiblememonly.

We could say that there are two distinct regions of inaccessible state, one for volatile operations, and one for inaccessiblememonly, but that doesn't seem like a good tradeoff. inaccessiblememonly is already an approximation for the sake of making it easily usable: inaccessiblememonly calls have a dependency on other, unrelated, inaccessiblememonly calls, even if they don't actually touch the same state. If there's some intrinsic that's particularly sensitive to this, we can refine alias analysis for that intrinsic, either with a new attribute or a special case written in C++.

Yes, volatile loads and stores access the "same" state as inaccessiblememonly functions, I think.

I agree. If we need finer granularity sets of inaccessible state, we can create that and figure out how to attach it at that point.

I think that, in general, we're on the same page. It seems a bit unfortunate that a volatile *load* can modify this inaccessible state. In practice, however, it can (e.g., some hardware uses loads from some address as an automatically-sliding-window over some larger internal register file), and without this, it would be difficult to maintain ordering. We should be explicit about this, however, in the text: volatile loads can modify his inaccessible state (which is consistent with the current implementation: mayWriteToMemory returns true for volatile loads).

Also, I think that we should be explicit about call-graph impacts. volatile accesses may have target-specific side effects, but these don't include calling functions that are part of the module being compiled.

efriedma updated this revision to Diff 181940.Jan 15 2019, 5:18 PM

Updated wording to clarify that volatile operations can modify inaccessible state, but not accessible state (except for the address of the operation). Clarify that all volatile operations can have side-effects, and can read and modify inaccessible state.

hfinkel accepted this revision.Jan 21 2019, 4:19 PM

Ping

LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.
jfb added a subscriber: jfb.Jan 28 2019, 11:34 AM
jfb added inline comments.
llvm/trunk/docs/LangRef.rst
2196

Sorry for missing the initial discussion. This disallows using mmap to map the same physical memory into two different virtual addresses in the same process. In C++ volatile loads and stores exist to denote "external modifications", where "external" is w.r.t. the abstract machine. It seems like a misstep to say that "A volatile operation may not modify any other memory accessible by the module being compiled."

hfinkel added inline comments.Jan 28 2019, 12:26 PM
llvm/trunk/docs/LangRef.rst
2196

There are certainly some things that you can arrange using mmap, signal handlers, and other system calls that fall outside of the model that we support. This is one of them. To put it another way, if you do this, then you'd need to access all copies with volatile accesses. Accessing some copies as volatile, and some copies not, isn't something I can see how we can support (otherwise, we'd need to assume that all memory was modified by all volatile accesses to any address, and the cost of that is certainly not worth the benefits).

Please feel free to suggest language to make all of this clearer.

jfb added inline comments.Jan 28 2019, 12:35 PM
llvm/trunk/docs/LangRef.rst
2196

Agreed that both sides, reader and writer, need to use volatile. It seems like the current wording doesn't allow for this. From your response it sounds like it was an oversight? If so I can indeed send a follow-up.

hfinkel added inline comments.Jan 28 2019, 12:42 PM
llvm/trunk/docs/LangRef.rst
2196

From my perspective, if you use mmap to map some backing store to multiple places in the address space, then that's not "memory" as is intended by the statement (it's something else, like MMIO registers) - it's something you essentially cannot access using regular memory accesses. We might certainly be able to make the text clearer, or more carefully classify things in this regard.