Skip to content

Commit

Permalink
[FunctionAttrs] Volatile operations can access inaccessible memory
Browse files Browse the repository at this point in the history
Per LangRef, volatile operations are allowed to access the location
of their pointer argument, plus inaccessible memory:

> Any volatile operation can have side effects, and any volatile
> operation can read and/or modify state which is not accessible
> via a regular load or store in this module.
> [...]
> The allowed side-effects for volatile accesses are limited. If
> a non-volatile store to a given address would be legal, a volatile
> operation may modify the memory at that address. A volatile
> operation may not modify any other memory accessible by the
> module being compiled. A volatile operation may not call any
> code in the current module.

FunctionAttrs currently does not model this and ends up marking
functions with volatile accesses on arguments as argmemonly,
even though they should be inaccessiblemem_or_argmemonly.

Differential Revision: https://reviews.llvm.org/D135863
  • Loading branch information
nikic committed Oct 20, 2022
1 parent 0f4dc56 commit f2fe289
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 10 deletions.
8 changes: 6 additions & 2 deletions llvm/lib/Transforms/IPO/FunctionAttrs.cpp
Expand Up @@ -217,8 +217,12 @@ static MemoryEffects checkFunctionMemoryAccess(Function &F, bool ThisBody,
continue;
}

// Ignore non-volatile accesses from local memory. (Atomic is okay here.)
if (!I.isVolatile() && AAR.pointsToConstantMemory(*Loc, /*OrLocal=*/true))
// Volatile operations may access inaccessible memory.
if (I.isVolatile())
ME |= MemoryEffects::inaccessibleMemOnly(MR);

// Ignore accesses to local memory.
if (AAR.pointsToConstantMemory(*Loc, /*OrLocal=*/true))
continue;

// The accessed location can be either only argument memory, or
Expand Down
12 changes: 6 additions & 6 deletions llvm/test/Transforms/FunctionAttrs/nosync.ll
Expand Up @@ -161,7 +161,7 @@ define void @store_unordered(ptr nocapture %0) norecurse nounwind uwtable {
; negative, should not deduce nosync
; atomic load with release ordering
define void @load_release(ptr nocapture %0) norecurse nounwind uwtable {
; CHECK: Function Attrs: argmemonly nofree norecurse nounwind uwtable
; CHECK: Function Attrs: inaccessiblemem_or_argmemonly nofree norecurse nounwind uwtable
; CHECK-LABEL: @load_release(
; CHECK-NEXT: store atomic volatile i32 10, ptr [[TMP0:%.*]] release, align 4
; CHECK-NEXT: ret void
Expand All @@ -172,7 +172,7 @@ define void @load_release(ptr nocapture %0) norecurse nounwind uwtable {

; negative volatile, relaxed atomic
define void @load_volatile_release(ptr nocapture %0) norecurse nounwind uwtable {
; CHECK: Function Attrs: argmemonly nofree norecurse nounwind uwtable
; CHECK: Function Attrs: inaccessiblemem_or_argmemonly nofree norecurse nounwind uwtable
; CHECK-LABEL: @load_volatile_release(
; CHECK-NEXT: store atomic volatile i32 10, ptr [[TMP0:%.*]] release, align 4
; CHECK-NEXT: ret void
Expand All @@ -183,7 +183,7 @@ define void @load_volatile_release(ptr nocapture %0) norecurse nounwind uwtable

; volatile store.
define void @volatile_store(ptr %0) norecurse nounwind uwtable {
; CHECK: Function Attrs: argmemonly nofree norecurse nounwind uwtable
; CHECK: Function Attrs: inaccessiblemem_or_argmemonly nofree norecurse nounwind uwtable
; CHECK-LABEL: @volatile_store(
; CHECK-NEXT: store volatile i32 14, ptr [[TMP0:%.*]], align 4
; CHECK-NEXT: ret void
Expand All @@ -195,7 +195,7 @@ define void @volatile_store(ptr %0) norecurse nounwind uwtable {
; negative, should not deduce nosync
; volatile load.
define i32 @volatile_load(ptr %0) norecurse nounwind uwtable {
; CHECK: Function Attrs: argmemonly mustprogress nofree norecurse nounwind willreturn uwtable
; CHECK: Function Attrs: inaccessiblemem_or_argmemonly mustprogress nofree norecurse nounwind willreturn uwtable
; CHECK-LABEL: @volatile_load(
; CHECK-NEXT: [[TMP2:%.*]] = load volatile i32, ptr [[TMP0:%.*]], align 4
; CHECK-NEXT: ret i32 [[TMP2]]
Expand All @@ -211,7 +211,7 @@ declare void @nosync_function() noinline nounwind uwtable nosync
define void @call_nosync_function() nounwind uwtable noinline {
; CHECK: Function Attrs: noinline nosync nounwind uwtable
; CHECK-LABEL: @call_nosync_function(
; CHECK-NEXT: tail call void @nosync_function() #[[ATTR10:[0-9]+]]
; CHECK-NEXT: tail call void @nosync_function() #[[ATTR11:[0-9]+]]
; CHECK-NEXT: ret void
;
tail call void @nosync_function() noinline nounwind uwtable
Expand All @@ -225,7 +225,7 @@ declare void @might_sync() noinline nounwind uwtable
define void @call_might_sync() nounwind uwtable noinline {
; CHECK: Function Attrs: noinline nounwind uwtable
; CHECK-LABEL: @call_might_sync(
; CHECK-NEXT: tail call void @might_sync() #[[ATTR10]]
; CHECK-NEXT: tail call void @might_sync() #[[ATTR11]]
; CHECK-NEXT: ret void
;
tail call void @might_sync() noinline nounwind uwtable
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Transforms/FunctionAttrs/readattrs.ll
Expand Up @@ -175,7 +175,7 @@ define <4 x i32> @test12_2(<4 x ptr> %ptrs) {
}

define i32 @volatile_load(ptr %p) {
; CHECK: Function Attrs: argmemonly mustprogress nofree norecurse nounwind willreturn
; CHECK: Function Attrs: inaccessiblemem_or_argmemonly mustprogress nofree norecurse nounwind willreturn
; CHECK-LABEL: define {{[^@]+}}@volatile_load
; CHECK-SAME: (ptr [[P:%.*]]) #[[ATTR13:[0-9]+]] {
; CHECK-NEXT: [[LOAD:%.*]] = load volatile i32, ptr [[P]], align 4
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Transforms/FunctionAttrs/writeonly.ll
Expand Up @@ -96,7 +96,7 @@ define void @test_readwrite(ptr %p) {
}

define void @test_volatile(ptr %p) {
; CHECK: Function Attrs: argmemonly nofree norecurse nounwind
; CHECK: Function Attrs: inaccessiblemem_or_argmemonly nofree norecurse nounwind
; CHECK-LABEL: define {{[^@]+}}@test_volatile
; CHECK-SAME: (ptr [[P:%.*]]) #[[ATTR6:[0-9]+]] {
; CHECK-NEXT: store volatile i8 0, ptr [[P]], align 1
Expand Down

0 comments on commit f2fe289

Please sign in to comment.