[FunctionAttrs][IR] Fix memory attr inference for volatile mem intrinsics #122926
Conversation
…sics Per LangRef volatile operations can read and write inaccessible memory: > any volatile operation can read and/or modify state which is not > accessible via a regular load or store in this module Model this by adding inaccessible memory effects in getMemoryEffects() if the operation is volatile. Fixes llvm#120932.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesPer LangRef volatile operations can read and write inaccessible memory: > any volatile operation can read and/or modify state which is not Model this by adding inaccessible memory effects in getMemoryEffects() if the operation is volatile. Fixes #120932. Full diff: https://github.com/llvm/llvm-project/pull/122926.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index b2a3f3390e000d..2bed294c607768 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -824,6 +824,10 @@ MemoryEffects BasicAAResult::getMemoryEffects(const CallBase *Call,
FuncME |= MemoryEffects::readOnly();
if (Call->hasClobberingOperandBundles())
FuncME |= MemoryEffects::writeOnly();
+ if (Call->isVolatile()) {
+ // Volatile operations also access inaccessible memory.
+ FuncME |= MemoryEffects::inaccessibleMemOnly();
+ }
Min &= FuncME;
}
diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp
index b8b2c1d7f9a859..3c69d82dfda073 100644
--- a/llvm/lib/IR/Instructions.cpp
+++ b/llvm/lib/IR/Instructions.cpp
@@ -615,6 +615,10 @@ MemoryEffects CallBase::getMemoryEffects() const {
if (hasClobberingOperandBundles())
FnME |= MemoryEffects::writeOnly();
}
+ if (isVolatile()) {
+ // Volatile operations also access inaccessible memory.
+ FnME |= MemoryEffects::inaccessibleMemOnly();
+ }
ME &= FnME;
}
return ME;
diff --git a/llvm/test/Transforms/FunctionAttrs/initializes.ll b/llvm/test/Transforms/FunctionAttrs/initializes.ll
index 62f217aa3f7de9..b2e6c284747eb7 100644
--- a/llvm/test/Transforms/FunctionAttrs/initializes.ll
+++ b/llvm/test/Transforms/FunctionAttrs/initializes.ll
@@ -443,7 +443,7 @@ define void @memset_neg(ptr %p) {
}
define void @memset_volatile(ptr %p) {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: write)
+; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: write, inaccessiblemem: readwrite)
; CHECK-LABEL: define void @memset_volatile(
; CHECK-SAME: ptr writeonly [[P:%.*]]) #[[ATTR5:[0-9]+]] {
; CHECK-NEXT: call void @llvm.memset.p0.i64(ptr [[P]], i8 2, i64 9, i1 true)
@@ -478,7 +478,7 @@ define void @memcpy(ptr %p, ptr %p2) {
}
define void @memcpy_volatile(ptr %p, ptr %p2) {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: readwrite)
+; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: readwrite, inaccessiblemem: readwrite)
; CHECK-LABEL: define void @memcpy_volatile(
; CHECK-SAME: ptr writeonly [[P:%.*]], ptr readonly [[P2:%.*]]) #[[ATTR6:[0-9]+]] {
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr [[P]], ptr [[P2]], i64 9, i1 true)
@@ -541,7 +541,7 @@ define void @memmove(ptr %p, ptr %p2) {
}
define void @memmove_volatile(ptr %p, ptr %p2) {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: readwrite)
+; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: readwrite, inaccessiblemem: readwrite)
; CHECK-LABEL: define void @memmove_volatile(
; CHECK-SAME: ptr writeonly [[P:%.*]], ptr readonly [[P2:%.*]]) #[[ATTR6]] {
; CHECK-NEXT: call void @llvm.memmove.p0.p0.i64(ptr [[P]], ptr [[P2]], i64 9, i1 true)
diff --git a/llvm/test/Transforms/FunctionAttrs/nosync.ll b/llvm/test/Transforms/FunctionAttrs/nosync.ll
index de5398f17ce51d..9abfbb21a71a0a 100644
--- a/llvm/test/Transforms/FunctionAttrs/nosync.ll
+++ b/llvm/test/Transforms/FunctionAttrs/nosync.ll
@@ -236,7 +236,7 @@ declare void @llvm.memset(ptr %dest, i8 %val, i32 %len, i1 %isvolatile)
; negative, checking volatile intrinsics.
define i32 @memcpy_volatile(ptr %ptr1, ptr %ptr2) {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: readwrite)
+; CHECK: Function Attrs: mustprogress nofree norecurse nounwind willreturn memory(argmem: readwrite, inaccessiblemem: readwrite)
; CHECK-LABEL: @memcpy_volatile(
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr [[PTR1:%.*]], ptr [[PTR2:%.*]], i32 8, i1 true)
; CHECK-NEXT: ret i32 4
|
There was a problem hiding this comment.
Why aren't the function declarations correct in the first place? If llvm.memcpy can be volatile, the intrinsic declaration should indicate that it can modify inaccessible memory. Marking the declaration argmemonly, then overriding that later, seems backwards.
|
Yes, our modelling of volatile memory intrinsics is generally broken -- but I don't think there's a lot of value in changing the attributes on the declaration. Ultimately we'll still have to fix things up, just in the opposite direction of what we do now. If we really want to improve things, we should split off separate llvm.memset.volatile etc intrinsics. That way everything gets correct memory modelling and all optimizations will implicitly ignore the volatile variants by default. |
There was a problem hiding this comment.
The primary value of fixing the attributes on the declarations would be to reduce the likelihood of mistakes: if someone accidentally queries the attributes on llvm.memcpy directly, instead of going through the CallBase wrappers, they get the conservatively right result. Otherwise it would be basically the same as this patch.
I'm not sure I see the value in splitting all the memory intrinsics into volatile/non-volatile versions; that seems like a lot of redundancy. I guess most places don't use the names of memory intrinsics directly anyway, so maybe it's not that bad.
|
I've been thinking about this again, and I think a clean way to handle this may be via operand bundle. ; Non-volatile memset
call void @llvm.memset(ptr %p, i8 0, i64 %len)
; Volatile memset
call void @llvm.memset(ptr %p, i8 0, i64 %len) ["volatile"()]Operand bundles are allowed to add additional memory effects, so this would be consistent with our overall IR model. |
|
Sure, I think that makes sense. |
Per LangRef volatile operations can read and write inaccessible memory:
Model this by adding inaccessible memory effects in getMemoryEffects() if the operation is volatile.
Fixes #120932.