Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir][LLVM] Model side effects of volatile and atomic load-store #65730

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

zero9178
Copy link
Member

@zero9178 zero9178 commented Sep 8, 2023

According to the LLVM language reference, both volatile memory operations and atomic operations (except unordered) do not simply read memory but also perform write operations on arbitrary memory[0][1].

In the case of volatile memory operations, this is the case due to the read possibly having target specific properties. A common real-world situation where this happens is reading memory mapped registers on an MCU for example. Atomic operations are more special. They form a kind of memory barrier which from the perspective of the optimizer/lang-ref makes writes from other threads visible in the current thread. Any kind of synchronization can therefore conservatively be modeled as a write-effect.

This PR therefore adjusts the side effects of llvm.load and llvm.store to add unknown global read and write effects if they are either atomic or volatile.

Regarding testing: I am not sure how to best test this change for llvm.store and the "globalness" of the effect that isn't just a unit test checking that the output matches exactly. For the time being, I added a test making sure that llvm.load does not get DCEd in aforementioned cases.

Related logic in LLVM proper:

bool Instruction::mayReadFromMemory() const {
switch (getOpcode()) {
default: return false;
case Instruction::VAArg:
case Instruction::Load:
case Instruction::Fence: // FIXME: refine definition of mayReadFromMemory
case Instruction::AtomicCmpXchg:
case Instruction::AtomicRMW:
case Instruction::CatchPad:
case Instruction::CatchRet:
return true;
case Instruction::Call:
case Instruction::Invoke:
case Instruction::CallBr:
return !cast<CallBase>(this)->onlyWritesMemory();
case Instruction::Store:
return !cast<StoreInst>(this)->isUnordered();
}
}
bool Instruction::mayWriteToMemory() const {
switch (getOpcode()) {
default: return false;
case Instruction::Fence: // FIXME: refine definition of mayWriteToMemory
case Instruction::Store:
case Instruction::VAArg:
case Instruction::AtomicCmpXchg:
case Instruction::AtomicRMW:
case Instruction::CatchPad:
case Instruction::CatchRet:
return true;
case Instruction::Call:
case Instruction::Invoke:
case Instruction::CallBr:
return !cast<CallBase>(this)->onlyReadsMemory();
case Instruction::Load:
return !cast<LoadInst>(this)->isUnordered();
}
}

bool isUnordered() const {
return (getOrdering() == AtomicOrdering::NotAtomic ||
getOrdering() == AtomicOrdering::Unordered) &&
!isVolatile();
}

[0] https://llvm.org/docs/LangRef.html#volatile-memory-accesses
[1] https://llvm.org/docs/Atomics.html#monotonic

According to the LLVM language reference, both volatile memory operations and atomic operations (except unordered) do not simply read memory but also perform write operations on arbitrary memory[0][1].

In the case of volatile memory operations, this is the case due to the read possibly having target specific properties. A common real-world situation where this happens is reading memory mapped registers on an MCU for example. Atomic operations are more special. They form a kind of memory barrier which from the perspective of the optimizer/lang-ref makes writes from other threads visible in the current thread. Any kind of synchronization can therefore conservatively be modeled as a write-effect.

This PR therefore adjusts the side effects of `llvm.load` and `llvm.store` to add unknown global read and write effects if they are either atomic or volatile.

Regarding testing: I am not sure how to best test this change for `llvm.store` and the "globalness" of the effect that isn't just a unit test checking that the output matches exactly. For the time being, I added a test making sure that `llvm.load` does not get DCEd in aforementioned cases.

Related logic in LLVM proper:
https://github.com/llvm/llvm-project/blob/3398744a6106c83993611bd3c5e79ec6b94417dc/llvm/lib/IR/Instruction.cpp#L638-L676

https://github.com/llvm/llvm-project/blob/3398744a6106c83993611bd3c5e79ec6b94417dc/llvm/include/llvm/IR/Instructions.h#L258-L262

[0] https://llvm.org/docs/LangRef.html#volatile-memory-accesses
[1] https://llvm.org/docs/Atomics.html#monotonic
@zero9178 zero9178 requested a review from a team as a code owner September 8, 2023 09:23
@github-actions github-actions bot added mlir:core MLIR Core Infrastructure mlir labels Sep 8, 2023
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Adding a short comments that explain the additional effects would be nice!

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp Show resolved Hide resolved
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp Show resolved Hide resolved
@zero9178 zero9178 merged commit feb7bea into llvm:main Sep 8, 2023
2 checks passed
@zero9178 zero9178 deleted the volatile-side-effects branch September 8, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants