[FunctionAttrs][IR] Fix memory attr inference for volatile mem intrinsics #122926

nikic · 2025-01-14T15:55:21Z

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 #120932.

…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.

llvmbot · 2025-01-14T15:58:22Z

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-llvm-analysis

Author: Nikita Popov (nikic)

Changes

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 #120932.

Full diff: https://github.com/llvm/llvm-project/pull/122926.diff

4 Files Affected:

(modified) llvm/lib/Analysis/BasicAliasAnalysis.cpp (+4)
(modified) llvm/lib/IR/Instructions.cpp (+4)
(modified) llvm/test/Transforms/FunctionAttrs/initializes.ll (+3-3)
(modified) llvm/test/Transforms/FunctionAttrs/nosync.ll (+1-1)

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

efriedma-quic

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.

nikic · 2025-01-14T22:51:56Z

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.

efriedma-quic

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.

nikic · 2025-03-21T10:50:01Z

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.

efriedma-quic · 2025-03-23T17:43:56Z

Sure, I think that makes sense.

nikic · 2025-06-24T13:57:24Z

I'd still like to land this in the meantime. This is better than what we have now, even if it's not ideal.

efriedma-quic

LGTM. (It's not ideal, but it's clearly better than not checking for volatile at all.)

…sics (llvm#122926) 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. In the future, we should model volatile using operand bundles instead. Fixes llvm#120932.

nikic requested review from fhahn, dtcxzyw and efriedma-quic January 14, 2025 15:55

llvmbot added llvm:ir llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Jan 14, 2025

efriedma-quic reviewed Jan 14, 2025

View reviewed changes

nikic mentioned this pull request Jun 24, 2025

[EarlyCSE] Add support for writeonly call CSE #145474

Merged

efriedma-quic approved these changes Jun 24, 2025

View reviewed changes

nikic merged commit 7c38ee2 into llvm:main Jun 25, 2025
12 checks passed

nikic deleted the functionattrs-volatile-2 branch June 25, 2025 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FunctionAttrs][IR] Fix memory attr inference for volatile mem intrinsics #122926

[FunctionAttrs][IR] Fix memory attr inference for volatile mem intrinsics #122926

Uh oh!

nikic commented Jan 14, 2025

Uh oh!

llvmbot commented Jan 14, 2025 •

edited

Loading

Uh oh!

efriedma-quic left a comment

Uh oh!

nikic commented Jan 14, 2025

Uh oh!

efriedma-quic left a comment

Uh oh!

nikic commented Mar 21, 2025

Uh oh!

efriedma-quic commented Mar 23, 2025

Uh oh!

nikic commented Jun 24, 2025

Uh oh!

efriedma-quic left a comment

Uh oh!

Uh oh!

Uh oh!

[FunctionAttrs][IR] Fix memory attr inference for volatile mem intrinsics #122926

[FunctionAttrs][IR] Fix memory attr inference for volatile mem intrinsics #122926

Uh oh!

Conversation

nikic commented Jan 14, 2025

Uh oh!

llvmbot commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic commented Jan 14, 2025

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic commented Mar 21, 2025

Uh oh!

efriedma-quic commented Mar 23, 2025

Uh oh!

nikic commented Jun 24, 2025

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jan 14, 2025 •

edited

Loading