[PATCH] D137937: [TableGen] Represent IntrHasSideEffects using inaccessiblemem read+write

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: jdoerfert, nhaehnle, arsenm, foad.
Herald added subscribers: kosarev, kerbowa, tpr, jvesely.
Herald added a project: All.
nikic requested review of this revision.
Herald added subscribers: llvm-commits, wdng.
Herald added a project: LLVM.

Map IntrHasSideEffects to a read+write of inaccessible memory, which is the usual way to represent a side effect that cannot be modelled more precisely.

This means that IntrNoMem + IntrHasSideEffects is reduced from reading and writing all memory to only accessing inaccessible memory, while IntrReadMem + IntrHasSideEffects is now no longer readonly (which is clearly incorrect as it e.g. allows simply removing the side effect).

The fact that `IntrNoMem + IntrHasSideEffects` is no longer an arbitrary read and write may cause issues for existing intrinsics marked as such. In particular, I think the `llvm.amdgcn.s.barrier` test changes here look incorrect -- if I understand the semantics of that intrinsic correctly, then moving loads/stores across the intrinsic is not legal, and the current IntrNoMem marking is simply incorrect. Can somebody from the AMDGPU side please confirm what the intended semantics are?


https://reviews.llvm.org/D137937

Files:
  llvm/test/Analysis/GlobalsModRef/nosync_nocallback.ll
  llvm/test/CodeGen/AMDGPU/noclobber-barrier.ll
  llvm/test/TableGen/intrin-side-effects.td
  llvm/test/Transforms/OpenMP/barrier_removal.ll
  llvm/utils/TableGen/IntrinsicEmitter.cpp


Index: llvm/utils/TableGen/IntrinsicEmitter.cpp
===================================================================
--- llvm/utils/TableGen/IntrinsicEmitter.cpp
+++ llvm/utils/TableGen/IntrinsicEmitter.cpp
@@ -773,9 +773,10 @@
       OS << "      Attribute::get(C, Attribute::Speculatable),\n";
 
     MemoryEffects ME = Intrinsic.ME;
-    // TODO: IntrHasSideEffects should affect not only readnone intrinsics.
-    if (ME.doesNotAccessMemory() && Intrinsic.hasSideEffects)
-      ME = MemoryEffects::unknown();
+    // Approximate side effects as a read and write of inaccessible memory.
+    // This imposes an ordering-constraint on side effects.
+    if (Intrinsic.hasSideEffects)
+      ME |= MemoryEffects::inaccessibleMemOnly();
     if (ME != MemoryEffects::unknown()) {
       OS << "      Attribute::getWithMemoryEffects(C, "
          << "MemoryEffects::createFromIntValue(" << ME.toIntValue() << ")),\n";
Index: llvm/test/Transforms/OpenMP/barrier_removal.ll
===================================================================
--- llvm/test/Transforms/OpenMP/barrier_removal.ll
+++ llvm/test/Transforms/OpenMP/barrier_removal.ll
@@ -246,7 +246,7 @@
 ;.
 ; CHECK: attributes #[[ATTR0:[0-9]+]] = { "llvm.assume"="ompx_aligned_barrier" }
 ; CHECK: attributes #[[ATTR1:[0-9]+]] = { convergent nocallback nounwind }
-; CHECK: attributes #[[ATTR2:[0-9]+]] = { convergent nounwind willreturn }
+; CHECK: attributes #[[ATTR2:[0-9]+]] = { convergent nounwind willreturn memory(inaccessiblemem: readwrite) }
 ;.
 ; CHECK: [[META0:![0-9]+]] = !{i32 7, !"openmp", i32 50}
 ; CHECK: [[META1:![0-9]+]] = !{i32 7, !"openmp-device", i32 50}
Index: llvm/test/TableGen/intrin-side-effects.td
===================================================================
--- llvm/test/TableGen/intrin-side-effects.td
+++ llvm/test/TableGen/intrin-side-effects.td
@@ -44,6 +44,7 @@
 // CHECK: case 0:
 // CHECK-NEXT: return AttributeSet::get(C, {
 // CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(12)),
 // CHECK-NEXT: });
 
 // CHECK: 1, // llvm.random.gen
Index: llvm/test/CodeGen/AMDGPU/noclobber-barrier.ll
===================================================================
--- llvm/test/CodeGen/AMDGPU/noclobber-barrier.ll
+++ llvm/test/CodeGen/AMDGPU/noclobber-barrier.ll
@@ -192,7 +192,7 @@
 }
 
 ; GCN-LABEL: {{^}}no_clobbering_loop1:
-; GCN: s_load_dword s
+; GCN: s_load_dwordx2 s
 ; GCN: s_load_dword s
 ; GCN-NOT: global_load_dword
 ; GCN: global_store_dword
Index: llvm/test/Analysis/GlobalsModRef/nosync_nocallback.ll
===================================================================
--- llvm/test/Analysis/GlobalsModRef/nosync_nocallback.ll
+++ llvm/test/Analysis/GlobalsModRef/nosync_nocallback.ll
@@ -12,13 +12,16 @@
 define void @test_barrier(i1 %c) {
 ; CHECK-LABEL: define {{[^@]+}}@test_barrier
 ; CHECK-SAME: (i1 [[C:%.*]]) {
-; CHECK-NEXT:    br i1 [[C]], label [[INIT:%.*]], label [[CHECK:%.*]]
+; CHECK-NEXT:    br i1 [[C]], label [[INIT:%.*]], label [[DOTCHECK_CRIT_EDGE:%.*]]
+; CHECK:       .check_crit_edge:
+; CHECK-NEXT:    [[V_PRE:%.*]] = load i32, ptr @G1, align 4
+; CHECK-NEXT:    br label [[CHECK:%.*]]
 ; CHECK:       init:
 ; CHECK-NEXT:    store i32 0, ptr @G1, align 4
 ; CHECK-NEXT:    br label [[CHECK]]
 ; CHECK:       check:
+; CHECK-NEXT:    [[V:%.*]] = phi i32 [ [[V_PRE]], [[DOTCHECK_CRIT_EDGE]] ], [ 0, [[INIT]] ]
 ; CHECK-NEXT:    call void @llvm.amdgcn.s.barrier()
-; CHECK-NEXT:    [[V:%.*]] = load i32, ptr @G1, align 4
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[V]], 0
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[CMP]])
 ; CHECK-NEXT:    ret void


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137937.475113.patch
Type: text/x-patch
Size: 3682 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221114/29c637f6/attachment.bin>