[flang] Skip optimized bufferization on volatile refs

[ashermancinelli]

Memory effects on the volatile memory resource may not be attached to a particular source, in which case the value of an effect will be null. This caused this test case to crash in the optimized bufferization pass's safety analysis because it assumes it can get the SSA value modified by the memory effect. This is because memory effects on the volatile resource indicate that the operation must not be reordered with respect to other volatile operations, but there is not a material ssa value that can be pointed to.

This patch changes the safety checks such that memory effects which do not have associated values are not safe for optimized bufferization.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

Author: Asher Mancinelli (ashermancinelli)

Changes

Memory effects on the volatile memory resource may not be attached to a particular source, in which case the value of an effect will be null. This caused this test case to crash in the optimized bufferization pass's safety analysis because it assumes it can get the SSA value modified by the memory effect. This is because memory effects on the volatile resource indicate that the operation must not be reordered with respect to other volatile operations, but there is not a material ssa value that can be pointed to.

This patch changes the safety checks to indicate that that memory effects on volatile resources are not safe for optimized bufferization.

---

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

2 Files Affected:

• (modified) flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp (+7)
• (added) flang/test/HLFIR/opt-bufferization-skip-volatile.fir (+49)

diff --git a/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp b/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp
index 2f6ee2592a84f..120dc4c51f202 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp
@@ -608,6 +608,13 @@ ElementalAssignBufferization::findMatch(hlfir::ElementalOp elemental) {
       return std::nullopt;
     }
 
+    // Don't allow any reads to or writes from volatile memory
+    if (mlir::isa<mlir::MemoryEffects::Read, mlir::MemoryEffects::Write>(
+            effect.getEffect()) &&
+        mlir::isa<fir::VolatileMemoryResource>(effect.getResource())) {
+      return std::nullopt;
+    }
+
     // allow if and only if the reads are from the elemental indices, in order
     // => each iteration doesn't read values written by other iterations
     // don't allow reads from a different value which may alias: fir alias
diff --git a/flang/test/HLFIR/opt-bufferization-skip-volatile.fir b/flang/test/HLFIR/opt-bufferization-skip-volatile.fir
new file mode 100644
index 0000000000000..158f92bf207d2
--- /dev/null
+++ b/flang/test/HLFIR/opt-bufferization-skip-volatile.fir
@@ -0,0 +1,49 @@
+// RUN: fir-opt --pass-pipeline="builtin.module(func.func(opt-bufferization))" %s | FileCheck %s
+
+// Ensure optimized bufferization preserves the semantics of volatile arrays
+func.func @minimal_volatile_test() {
+  %c1 = arith.constant 1 : index
+  %c200 = arith.constant 200 : index
+  
+  // Create a volatile array
+  %1 = fir.address_of(@_QMtestEarray) : !fir.ref<!fir.array<200xf32>>
+  %2 = fir.shape %c200 : (index) -> !fir.shape<1>
+  %3 = fir.volatile_cast %1 : (!fir.ref<!fir.array<200xf32>>) -> !fir.ref<!fir.array<200xf32>, volatile>
+  %4:2 = hlfir.declare %3(%2) {fortran_attrs = #fir.var_attrs<volatile>, uniq_name = "_QMtestEarray"} : (!fir.ref<!fir.array<200xf32>, volatile>, !fir.shape<1>) -> (!fir.ref<!fir.array<200xf32>, volatile>, !fir.ref<!fir.array<200xf32>, volatile>)
+  
+  // Create an elemental operation that negates each element
+  %5 = hlfir.elemental %2 unordered : (!fir.shape<1>) -> !hlfir.expr<200xf32> {
+  ^bb0(%arg1: index):
+    %6 = hlfir.designate %4#0 (%arg1) : (!fir.ref<!fir.array<200xf32>, volatile>, index) -> !fir.ref<f32, volatile>
+    %7 = fir.load %6 : !fir.ref<f32, volatile>
+    %8 = arith.negf %7 : f32
+    hlfir.yield_element %8 : f32
+  }
+  
+  // Assign the result back to the volatile array
+  hlfir.assign %5 to %4#0 : !hlfir.expr<200xf32>, !fir.ref<!fir.array<200xf32>, volatile>
+  hlfir.destroy %5 : !hlfir.expr<200xf32>
+  
+  return
+}
+
+fir.global @_QMtestEarray : !fir.array<200xf32> 
+
+// CHECK-LABEL:   func.func @minimal_volatile_test() {
+// CHECK:           %[[VAL_0:.*]] = arith.constant 200 : index
+// CHECK:           %[[VAL_1:.*]] = fir.address_of(@_QMtestEarray) : !fir.ref<!fir.array<200xf32>>
+// CHECK:           %[[VAL_2:.*]] = fir.shape %[[VAL_0]] : (index) -> !fir.shape<1>
+// CHECK:           %[[VAL_3:.*]] = fir.volatile_cast %[[VAL_1]] : (!fir.ref<!fir.array<200xf32>>) -> !fir.ref<!fir.array<200xf32>, volatile>
+// CHECK:           %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_3]](%[[VAL_2]]) {fortran_attrs = #fir.var_attrs<volatile>, uniq_name = "_QMtestEarray"} : (!fir.ref<!fir.array<200xf32>, volatile>, !fir.shape<1>) -> (!fir.ref<!fir.array<200xf32>, volatile>, !fir.ref<!fir.array<200xf32>, volatile>)
+// CHECK:           %[[VAL_5:.*]] = hlfir.elemental %[[VAL_2]] unordered : (!fir.shape<1>) -> !hlfir.expr<200xf32> {
+// CHECK:           ^bb0(%[[VAL_6:.*]]: index):
+// CHECK:             %[[VAL_7:.*]] = hlfir.designate %[[VAL_4]]#0 (%[[VAL_6]])  : (!fir.ref<!fir.array<200xf32>, volatile>, index) -> !fir.ref<f32, volatile>
+// CHECK:             %[[VAL_8:.*]] = fir.load %[[VAL_7]] : !fir.ref<f32, volatile>
+// CHECK:             %[[VAL_9:.*]] = arith.negf %[[VAL_8]] : f32
+// CHECK:             hlfir.yield_element %[[VAL_9]] : f32
+// CHECK:           }
+// CHECK:           hlfir.assign %[[VAL_5]] to %[[VAL_4]]#0 : !hlfir.expr<200xf32>, !fir.ref<!fir.array<200xf32>, volatile>
+// CHECK:           hlfir.destroy %[[VAL_5]] : !hlfir.expr<200xf32>
+// CHECK:           return
+// CHECK:         }
+// CHECK:         fir.global @_QMtestEarray : !fir.array<200xf32>

[vzakhari]

Should not we instead check for getValue being nullptr at the places where it is used? It seems to be more generic than checking for volatile explicitly.

[ashermancinelli]

Should not we instead check for getValue being nullptr at the places where it is used? It seems to be more generic than checking for volatile explicitly.

I think you're right.

[jeanPerier]

Thanks for the fix!

Your fix makes sense to me as a general fix (we are probably lucky no operation with read effect to unknown value did not hit this before, they are probably not very common, or goes with write effects that made the code exit before).

I think there is still a case for doing optimized bufferization on volatile refs, but it is probably fine and safer to bail without more thinking.

Fortran does not really have a strong memory model like C++ that would allow precisely defining VOLATILE. The only thing the F2023 standard is saying in 8.5.20 is :

"The Fortran processor should use the most recent definition of a volatile object each time its value is required. When a volatile object is defined by means of Fortran, it should make that definition available to the non-Fortran parts of the program as soon as possible."

It is not clear to my that this should prevent us from the evaluating RHS of x = x + y directly into x if x or y is VOLATILE and there is no aliasing. VOLATILE is not mentioned in 15.5.2.14 as something allowing aliasing between Fortran entities, so I think it is still safe to assume that x and y do not overlap even when both are VOLATILE.

At least gfortran and ifx still optimize assignments when they are VOLATILE read/writes: https://godbolt.org/z/b9x317ezK

To clarify, I am not asking you to update the patch to ignore VOLATILE effects here (we would still need a bit more thinking to make sure things are OK for the cases more complex than x = x +y from my example). I just do not want future us to believe it would be incorrect to do so if we need do.

[tblah]

Thanks for the fix!

[ashermancinelli]

@jeanPerier

To clarify, I am not asking you to update the patch to ignore VOLATILE effects here (we would still need a bit more thinking to make sure things are OK for the cases more complex than x = x +y from my example). I just do not want future us to believe it would be incorrect to do so if we need do.

Thank you for the thoughtful review! I wonder if there's somewhere I should collect those ideas along with the design of volatile so we can iterate on it (or at least keep it documented somewhere). Maybe something in docs/?

[jeanPerier]

Thank you for the thoughtful review! I wonder if there's somewhere I should collect those ideas along with the design of volatile so we can iterate on it (or at least keep it documented somewhere). Maybe something in docs/?

Yes, that is a good idea.