[flang][nfc] Support volatility in Fir ops

[ashermancinelli]

Part two of merging #132486. Support volatility in fir ops.

• Introduce a new operation fir.volatile_cast, whose only purpose is to add or take away the volatility of an SSA value's type. The types must be otherwise identical, and any other type conversions must be handled by fir.convert. fir.convert will give an error if the volatility of the inputs does not match, such that all changes to volatility must be handled explicitly through fir.volatile_cast.
• Add memory effects to ops that read from or write to memory. The precedent for this comes from the LLVM dialect (feb7bea) where llvm.load/store ops with the volatile attribute report read/write effects to a generic memory resource. This change is similar in spirit but different in two ways: the volatility of an operation is determined by the type of its memref, not an attribute on the op, and the memory effects of a load- or store-like operation on a volatile reference type are reported against a particular memory resource, VolatileMemoryResource. This is so MLIR optimizations are able to reorder operations that are not volatile around operations that are, which we believe more precisely models LLVM's volatile memory semantics. @vzakhari suggested this in [flang] Add lowering of volatile references #132486 citing LangRef. See https://llvm.org/docs/LangRef.html#volatile-memory-accesses

Changes needed to generate IR with volatile types are not included in this change, so it should be non-functional, containing only the changes to Fir ops and op utilities that will be needed once we enable lowering to generate volatile types.

[llvmbot]

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

Author: Asher Mancinelli (ashermancinelli)

Changes

Part two of merging #132486. Support volatility in fir ops.

• Introduce a new operation fir.volatile_cast, whose only purpose is to add or take away the volatility of an SSA value's type. The types must be otherwise identical, and any other type conversions must be handled by fir.convert. fir.convert will give an error if the volatility of the inputs does not match, such that all changes to volatility must be handled explicitly through fir.volatile_cast.
• Add memory effects to ops that read from or write to memory. The precedent for this comes from the LLVM dialect (feb7bea) where llvm.load/store ops with the volatile attribute report read/write effects to a generic memory resource. This change is similar in spirit but different in two ways: the volatility of an operation is determined by the type of its memref, not an attribute on the op, and the memory effects of a load- or store-like operation on a volatile reference type are reported against a particular memory resource, VolatileMemoryResource. This is so MLIR optimizations are able to reorder operations that are not volatile around operations that are, which we believe more precisely models LLVM's volatile memory semantics. @vzakhari suggested this in #132486 citing LangRef. See https://llvm.org/docs/LangRef.html#volatile-memory-accesses

Changes needed to generate IR with volatile types are not included in this change, so it should be non-functional, containing only the changes to Fir ops and op utilities that will be needed once we enable lowering to generate volatile types.

---

Patch is 22.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134858.diff

9 Files Affected:

• (modified) flang/include/flang/Optimizer/Builder/FIRBuilder.h (+5)
• (modified) flang/include/flang/Optimizer/Dialect/FIROps.h (+6)
• (modified) flang/include/flang/Optimizer/Dialect/FIROps.td (+31-12)
• (modified) flang/include/flang/Optimizer/Dialect/FIROpsSupport.h (+19-7)
• (modified) flang/lib/Lower/ConvertExprToHLFIR.cpp (+7-2)
• (modified) flang/lib/Optimizer/Builder/FIRBuilder.cpp (+15-3)
• (modified) flang/lib/Optimizer/Dialect/FIROps.cpp (+97-1)
• (modified) flang/test/Fir/cse.fir (+20)
• (modified) flang/test/Fir/invalid.fir (+54)

diff --git a/flang/include/flang/Optimizer/Builder/FIRBuilder.h b/flang/include/flang/Optimizer/Builder/FIRBuilder.h
index ddd4ef7114a63..8fca4272636b1 100644
--- a/flang/include/flang/Optimizer/Builder/FIRBuilder.h
+++ b/flang/include/flang/Optimizer/Builder/FIRBuilder.h
@@ -397,6 +397,11 @@ class FirOpBuilder : public mlir::OpBuilder, public mlir::OpBuilder::Listener {
   mlir::Value createConvert(mlir::Location loc, mlir::Type toTy,
                             mlir::Value val);
 
+  /// Create a fir.convert op with a volatile cast if the source value's type
+  /// does not match the target type's volatility.
+  mlir::Value createConvertWithVolatileCast(mlir::Location loc, mlir::Type toTy,
+                                            mlir::Value val);
+
   /// Create a fir.store of \p val into \p addr. A lazy conversion
   /// of \p val to the element type of \p addr is created if needed.
   void createStoreWithConvert(mlir::Location loc, mlir::Value val,
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.h b/flang/include/flang/Optimizer/Dialect/FIROps.h
index ed301016ad01c..f3dbf47351ab8 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.h
@@ -50,6 +50,12 @@ struct DebuggingResource
   mlir::StringRef getName() final { return "DebuggingResource"; }
 };
 
+/// Model operations which read from/write to volatile memory
+struct VolatileMemoryResource
+    : public mlir::SideEffects::Resource::Base<VolatileMemoryResource> {
+  mlir::StringRef getName() final { return "VolatileMemoryResource"; }
+};
+
 class CoordinateIndicesAdaptor;
 using IntOrValue = llvm::PointerUnion<mlir::IntegerAttr, mlir::Value>;
 
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.td b/flang/include/flang/Optimizer/Dialect/FIROps.td
index 753e4bd18dc6d..c7ac2c0397413 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.td
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.td
@@ -286,7 +286,8 @@ def fir_FreeMemOp : fir_Op<"freemem", [MemoryEffects<[MemFree]>]> {
   let assemblyFormat = "$heapref attr-dict `:` qualified(type($heapref))";
 }
 
-def fir_LoadOp : fir_OneResultOp<"load", [FirAliasTagOpInterface]> {
+def fir_LoadOp : fir_OneResultOp<"load", [FirAliasTagOpInterface,
+                                          DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
   let summary = "load a value from a memory reference";
   let description = [{
     Load a value from a memory reference into an ssa-value (virtual register).
@@ -302,7 +303,7 @@ def fir_LoadOp : fir_OneResultOp<"load", [FirAliasTagOpInterface]> {
     or null.
   }];
 
-  let arguments = (ins Arg<AnyReferenceLike, "", [MemRead]>:$memref,
+  let arguments = (ins AnyReferenceLike:$memref,
                   OptionalAttr<LLVM_TBAATagArrayAttr>:$tbaa);
 
   let builders = [OpBuilder<(ins "mlir::Value":$refVal)>,
@@ -315,7 +316,8 @@ def fir_LoadOp : fir_OneResultOp<"load", [FirAliasTagOpInterface]> {
   }];
 }
 
-def fir_StoreOp : fir_Op<"store", [FirAliasTagOpInterface]> {
+def fir_StoreOp : fir_Op<"store", [FirAliasTagOpInterface,
+                                   DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
   let summary = "store an SSA-value to a memory location";
 
   let description = [{
@@ -335,7 +337,7 @@ def fir_StoreOp : fir_Op<"store", [FirAliasTagOpInterface]> {
   }];
 
   let arguments = (ins AnyType:$value,
-                   Arg<AnyReferenceLike, "", [MemWrite]>:$memref,
+                   AnyReferenceLike:$memref,
                    OptionalAttr<LLVM_TBAATagArrayAttr>:$tbaa);
 
   let builders = [OpBuilder<(ins "mlir::Value":$value, "mlir::Value":$memref)>];
@@ -348,7 +350,7 @@ def fir_StoreOp : fir_Op<"store", [FirAliasTagOpInterface]> {
   }];
 }
 
-def fir_CopyOp : fir_Op<"copy", []> {
+def fir_CopyOp : fir_Op<"copy", [DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
   let summary = "copy constant size memory";
 
   let description = [{
@@ -369,8 +371,8 @@ def fir_CopyOp : fir_Op<"copy", []> {
     TODO: add FirAliasTagOpInterface to carry TBAA.
   }];
 
-  let arguments = (ins Arg<AnyRefOfConstantSizeAggregateType, "", [MemRead]>:$source,
-                       Arg<AnyRefOfConstantSizeAggregateType, "", [MemWrite]>:$destination,
+  let arguments = (ins AnyRefOfConstantSizeAggregateType:$source,
+                       AnyRefOfConstantSizeAggregateType:$destination,
                        OptionalAttr<UnitAttr>:$no_overlap);
 
   let builders = [OpBuilder<(ins "mlir::Value":$source,
@@ -1373,7 +1375,8 @@ def fir_BoxTypeDescOp : fir_SimpleOneResultOp<"box_tdesc", [NoMemoryEffect]> {
 //   !- Merge the new and old values into the memory for "A"
 //   array_merge_store <updated A> to <A's address>
 
-def fir_ArrayLoadOp : fir_Op<"array_load", [AttrSizedOperandSegments]> {
+def fir_ArrayLoadOp : fir_Op<"array_load", [AttrSizedOperandSegments,
+                                            DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
 
   let summary = "Load an array as a value.";
 
@@ -1412,7 +1415,7 @@ def fir_ArrayLoadOp : fir_Op<"array_load", [AttrSizedOperandSegments]> {
   }];
 
   let arguments = (ins
-    Arg<AnyRefOrBox, "", [MemRead]>:$memref,
+    AnyRefOrBox:$memref,
     Optional<AnyShapeOrShiftType>:$shape,
     Optional<fir_SliceType>:$slice,
     Variadic<AnyIntegerType>:$typeparams
@@ -1624,7 +1627,7 @@ def fir_ArrayAccessOp : fir_Op<"array_access", [AttrSizedOperandSegments,
 
     It is only possible to use `array_access` on an `array_load` result value or
     a value that can be trace back transitively to an `array_load` as the
-    dominating source. Other array operation such as `array_amend` can be in
+    dominating source. Other array operations such as `array_amend` can be in
     between.
 
     TODO: The above restriction is not enforced. The design of the operation
@@ -1685,7 +1688,7 @@ def fir_ArrayAmendOp : fir_Op<"array_amend", [NoMemoryEffect]> {
 }
 
 def fir_ArrayMergeStoreOp : fir_Op<"array_merge_store",
-    [AttrSizedOperandSegments]> {
+    [AttrSizedOperandSegments, DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
 
   let summary = "Store merged array value to memory.";
 
@@ -1714,7 +1717,7 @@ def fir_ArrayMergeStoreOp : fir_Op<"array_merge_store",
   let arguments = (ins
     fir_SequenceType:$original,
     fir_SequenceType:$sequence,
-    Arg<AnyRefOrBox, "", [MemWrite]>:$memref,
+    AnyRefOrBox:$memref,
     Optional<fir_SliceType>:$slice,
     Variadic<AnyIntegerType>:$typeparams
   );
@@ -2752,6 +2755,22 @@ def fir_AddrOfOp : fir_OneResultOp<"address_of", [NoMemoryEffect]> {
   let assemblyFormat = "`(` $symbol `)` attr-dict `:` type($resTy)";
 }
 
+def fir_VolatileCastOp : fir_SimpleOneResultOp<"volatile_cast", [NoMemoryEffect]> {
+  let summary = "cast between volatile and non-volatile types";
+  let description = [{
+    Cast between volatile and non-volatile types. The types must be otherwise
+    identical. A value's volatility cannot be changed by a fir.convert operation.
+    Reinterpreting a value as volatile must be done explicitly using this operation.
+  }];
+  let arguments = (ins AnyRefOrBox:$value);
+  let results = (outs AnyRefOrBox:$res);
+  let assemblyFormat = [{
+    $value attr-dict `:` functional-type($value, results)
+  }];
+  let hasVerifier = 1;
+  let hasFolder = 1;
+}
+
 def fir_ConvertOp : fir_SimpleOneResultOp<"convert", [NoMemoryEffect]> {
   let summary = "encapsulates all Fortran entity type conversions";
 
diff --git a/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h b/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
index f7f0a3067b318..8f7ca63ca8d1f 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
@@ -15,13 +15,25 @@
 
 namespace fir {
 
-/// Return true iff the Operation is a non-volatile LoadOp or ArrayLoadOp.
-inline bool nonVolatileLoad(mlir::Operation *op) {
-  if (auto load = mlir::dyn_cast<fir::LoadOp>(op))
-    return !load->getAttr("volatile");
-  if (auto arrLoad = mlir::dyn_cast<fir::ArrayLoadOp>(op))
-    return !arrLoad->getAttr("volatile");
-  return false;
+/// The LLVM dialect represents volatile memory accesses as read and write
+/// effects to an unknown memory location, but this may be overly conservative.
+/// LLVM Language Reference only specifies that volatile memory accesses
+/// must not be reordered relative to other volatile memory accesses, so it
+/// is more precise to use a separate memory resource for volatile memory
+/// accesses.
+inline void addVolatileMemoryEffects(
+    mlir::TypeRange type,
+    llvm::SmallVectorImpl<
+        mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+        &effects) {
+  for (mlir::Type t : type) {
+    if (fir::isa_volatile_type(t)) {
+      effects.emplace_back(mlir::MemoryEffects::Read::get(),
+                           fir::VolatileMemoryResource::get());
+      effects.emplace_back(mlir::MemoryEffects::Write::get(),
+                           fir::VolatileMemoryResource::get());
+    }
+  }
 }
 
 /// Return true iff the Operation is a call.
diff --git a/flang/lib/Lower/ConvertExprToHLFIR.cpp b/flang/lib/Lower/ConvertExprToHLFIR.cpp
index 09bc1babf079c..8f5d799ea4c57 100644
--- a/flang/lib/Lower/ConvertExprToHLFIR.cpp
+++ b/flang/lib/Lower/ConvertExprToHLFIR.cpp
@@ -415,8 +415,13 @@ class HlfirDesignatorBuilder {
         .Case<fir::SequenceType>([&](fir::SequenceType seqTy) -> mlir::Type {
           return fir::SequenceType::get(seqTy.getShape(), newEleTy);
         })
-        .Case<fir::PointerType, fir::HeapType, fir::ReferenceType, fir::BoxType,
-              fir::ClassType>([&](auto t) -> mlir::Type {
+        .Case<fir::ReferenceType, fir::BoxType, fir::ClassType>(
+            [&](auto t) -> mlir::Type {
+              using FIRT = decltype(t);
+              return FIRT::get(changeElementType(t.getEleTy(), newEleTy),
+                               t.isVolatile());
+            })
+        .Case<fir::PointerType, fir::HeapType>([&](auto t) -> mlir::Type {
           using FIRT = decltype(t);
           return FIRT::get(changeElementType(t.getEleTy(), newEleTy));
         })
diff --git a/flang/lib/Optimizer/Builder/FIRBuilder.cpp b/flang/lib/Optimizer/Builder/FIRBuilder.cpp
index 7fc30ca125a87..cfacb4b47854f 100644
--- a/flang/lib/Optimizer/Builder/FIRBuilder.cpp
+++ b/flang/lib/Optimizer/Builder/FIRBuilder.cpp
@@ -577,6 +577,17 @@ mlir::Value fir::FirOpBuilder::convertWithSemantics(
   return createConvert(loc, toTy, val);
 }
 
+mlir::Value fir::FirOpBuilder::createConvertWithVolatileCast(mlir::Location loc,
+                                                             mlir::Type toTy,
+                                                             mlir::Value val) {
+  if (fir::isa_volatile_type(val.getType()) != fir::isa_volatile_type(toTy)) {
+    mlir::Type volatileAdjustedType = fir::updateTypeWithVolatility(
+        val.getType(), fir::isa_volatile_type(toTy));
+    val = create<fir::VolatileCastOp>(loc, volatileAdjustedType, val);
+  }
+  return createConvert(loc, toTy, val);
+}
+
 mlir::Value fir::factory::createConvert(mlir::OpBuilder &builder,
                                         mlir::Location loc, mlir::Type toTy,
                                         mlir::Value val) {
@@ -739,19 +750,20 @@ mlir::Value fir::FirOpBuilder::createBox(mlir::Location loc,
         << itemAddr.getType();
     llvm_unreachable("not a memory reference type");
   }
+  const bool isVolatile = fir::isa_volatile_type(itemAddr.getType());
   mlir::Type boxTy;
   mlir::Value tdesc;
   // Avoid to wrap a box/class with box/class.
   if (mlir::isa<fir::BaseBoxType>(elementType)) {
     boxTy = elementType;
   } else {
-    boxTy = fir::BoxType::get(elementType);
+    boxTy = fir::BoxType::get(elementType, isVolatile);
     if (isPolymorphic) {
       elementType = fir::updateTypeForUnlimitedPolymorphic(elementType);
       if (isAssumedType)
-        boxTy = fir::BoxType::get(elementType);
+        boxTy = fir::BoxType::get(elementType, isVolatile);
       else
-        boxTy = fir::ClassType::get(elementType);
+        boxTy = fir::ClassType::get(elementType, isVolatile);
     }
   }
 
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 2d8017d0318d2..4dc99ee00e298 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "flang/Optimizer/Dialect/FIROps.h"
+#include "flang/Optimizer/Builder/BoxValue.h"
 #include "flang/Optimizer/Dialect/FIRAttr.h"
 #include "flang/Optimizer/Dialect/FIRDialect.h"
 #include "flang/Optimizer/Dialect/FIROpsSupport.h"
@@ -29,6 +30,7 @@
 #include "mlir/IR/Matchers.h"
 #include "mlir/IR/OpDefinition.h"
 #include "mlir/IR/PatternMatch.h"
+#include "mlir/IR/TypeRange.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/TypeSwitch.h"
@@ -853,6 +855,16 @@ std::vector<mlir::Value> fir::ArrayLoadOp::getExtents() {
   return {};
 }
 
+void fir::ArrayLoadOp::getEffects(
+    llvm::SmallVectorImpl<
+        mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+        &effects) {
+  effects.emplace_back(mlir::MemoryEffects::Read::get(),
+                       &getOperation()->getOpOperand(0),
+                       mlir::SideEffects::DefaultResource::get());
+  addVolatileMemoryEffects({getMemref().getType()}, effects);
+}
+
 llvm::LogicalResult fir::ArrayLoadOp::verify() {
   auto eleTy = fir::dyn_cast_ptrOrBoxEleTy(getMemref().getType());
   auto arrTy = mlir::dyn_cast<fir::SequenceType>(eleTy);
@@ -935,6 +947,16 @@ llvm::LogicalResult fir::ArrayMergeStoreOp::verify() {
   return mlir::success();
 }
 
+void fir::ArrayMergeStoreOp::getEffects(
+    llvm::SmallVectorImpl<
+        mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+        &effects) {
+  effects.emplace_back(mlir::MemoryEffects::Write::get(),
+                       &getOperation()->getOpOperand(0),
+                       mlir::SideEffects::DefaultResource::get());
+  addVolatileMemoryEffects({getMemref().getType()}, effects);
+}
+
 //===----------------------------------------------------------------------===//
 // ArrayFetchOp
 //===----------------------------------------------------------------------===//
@@ -1322,6 +1344,36 @@ mlir::ParseResult fir::CmpcOp::parse(mlir::OpAsmParser &parser,
   return parseCmpOp<fir::CmpcOp>(parser, result);
 }
 
+//===----------------------------------------------------------------------===//
+// VolatileCastOp
+//===----------------------------------------------------------------------===//
+
+llvm::LogicalResult fir::VolatileCastOp::verify() {
+  mlir::Type fromType = getValue().getType();
+  mlir::Type toType = getType();
+  // Other than volatility, are the types identical?
+  const bool sameBaseType =
+      llvm::TypeSwitch<mlir::Type, bool>(fromType)
+          .Case<fir::BoxType, fir::ReferenceType, fir::ClassType>(
+              [&](auto type) {
+                using TYPE = decltype(type);
+                return mlir::isa<TYPE>(toType);
+              })
+          .Default([=](mlir::Type) { return fromType == toType; });
+  const bool sameElementType = fir::dyn_cast_ptrOrBoxEleTy(fromType) ==
+                               fir::dyn_cast_ptrOrBoxEleTy(toType);
+  if (!sameBaseType || !sameElementType)
+    return emitOpError("types must be identical except for volatility ")
+           << fromType << " / " << toType;
+  return mlir::success();
+}
+
+mlir::OpFoldResult fir::VolatileCastOp::fold(FoldAdaptor adaptor) {
+  if (getValue().getType() == getType())
+    return getValue();
+  return {};
+}
+
 //===----------------------------------------------------------------------===//
 // ConvertOp
 //===----------------------------------------------------------------------===//
@@ -1461,7 +1513,13 @@ bool fir::ConvertOp::canBeConverted(mlir::Type inType, mlir::Type outType) {
 }
 
 llvm::LogicalResult fir::ConvertOp::verify() {
-  if (canBeConverted(getValue().getType(), getType()))
+  mlir::Type inType = getValue().getType();
+  mlir::Type outType = getType();
+  if (fir::isa_volatile_type(inType) != fir::isa_volatile_type(outType))
+    return emitOpError("cannot convert between volatile and non-volatile "
+                       "types, use fir.volatile_cast instead ")
+           << inType << " / " << outType;
+  if (canBeConverted(inType, outType))
     return mlir::success();
   return emitOpError("invalid type conversion")
          << getValue().getType() << " / " << getType();
@@ -1787,6 +1845,10 @@ llvm::LogicalResult fir::EmboxOp::verify() {
     return emitOpError("slice must not be provided for a scalar");
   if (getSourceBox() && !mlir::isa<fir::ClassType>(getResult().getType()))
     return emitOpError("source_box must be used with fir.class result type");
+  if (fir::isa_volatile_type(getMemref().getType()) !=
+      fir::isa_volatile_type(getResult().getType()))
+    return emitOpError("cannot convert between volatile and non-volatile "
+                       "types, use fir.volatile_cast instead");
   return mlir::success();
 }
 
@@ -2599,6 +2661,16 @@ void fir::LoadOp::print(mlir::OpAsmPrinter &p) {
   p << " : " << getMemref().getType();
 }
 
+void fir::LoadOp::getEffects(
+    llvm::SmallVectorImpl<
+        mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+        &effects) {
+  effects.emplace_back(mlir::MemoryEffects::Read::get(),
+                       &getOperation()->getOpOperand(0),
+                       mlir::SideEffects::DefaultResource::get());
+  addVolatileMemoryEffects({getMemref().getType()}, effects);
+}
+
 //===----------------------------------------------------------------------===//
 // DoLoopOp
 //===----------------------------------------------------------------------===//
@@ -3951,6 +4023,16 @@ void fir::StoreOp::build(mlir::OpBuilder &builder, mlir::OperationState &result,
   build(builder, result, value, memref, {});
 }
 
+void fir::StoreOp::getEffects(
+    llvm::SmallVectorImpl<
+        mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+        &effects) {
+  effects.emplace_back(mlir::MemoryEffects::Write::get(),
+                       &getOperation()->getOpOperand(1),
+                       mlir::SideEffects::DefaultResource::get());
+  addVolatileMemoryEffects({getMemref().getType()}, effects);
+}
+
 //===----------------------------------------------------------------------===//
 // CopyOp
 //===----------------------------------------------------------------------===//
@@ -3971,6 +4053,20 @@ llvm::LogicalResult fir::CopyOp::verify() {
   return mlir::success();
 }
 
+void fir::CopyOp::getEffects(
+    llvm::SmallVectorImpl<
+        mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+        &effects) {
+  effects.emplace_back(mlir::MemoryEffects::Read::get(),
+                       &getOperation()->getOpOperand(0),
+                       mlir::SideEffects::DefaultResource::get());
+  effects.emplace_back(mlir::MemoryEffects::Write::get(),
+                       &getOperation()->getOpOperand(1),
+                       mlir::SideEffects::DefaultResource::get());
+  addVolatileMemoryEffects({getDestination().getType(), getSource().getType()},
+                           effects);
+}
+
 //===----------------------------------------------------------------------===//
 // StringLitOp
 //===----------------------------------------------------------------------===//
diff --git a/flang/test/Fir/cse.fir b/flang/test/Fir/cse.fir
index 8813b7c411f50..590a9681f7405 100644
--- a/flang/test/Fir/cse.fir
+++ b/flang/test/Fir/cse.fir
@@ -55,3 +55,23 @@ func.func @fun(%a : !fir.ref<i64>) -> i64 {
   %5 = arith.subi %4, %4 : i64
   return %5 : i64
 }
+
+// -----
+
+// Check that the redundant ops on volatile operands are PRESERVED.
+func.func @fun(%arg0: !fir.ref<i64, volatile>) -> i64 {
+    %0 = fir.load %arg0 : !fir.ref<i64, volatile>
+    %1 = fir.load %arg0 : !fir.ref<i64, volatile>
+    %2 = arith.addi %0, %1 : i64
+    fir.store %2 to %arg0 : !fir.ref<i64, volatile>
+    fir.store %2 to %arg0 : !fir.ref<i64, volatile>
+    return %2 : i64
+}
+// CHECK-LABEL:   func.func @fun(%arg0: !fir.ref<i64, volatile>) -> i64 {
+// CHECK:           %[[VAL_1:.*]] = fir.load %arg0 : !fir.ref<i6...
[truncated]

[clementval]

LGTM

[tblah]

Thanks for this. Only minor comments

[ashermancinelli]

Thank you for the review @tblah!

[tblah]

LGTM. Thanks for the update

[llvm-ci]

LLVM Buildbot has detected a new failure on builder flang-aarch64-sharedlibs running on linaro-flang-aarch64-sharedlibs while building flang at step 5 "build-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/80/builds/12361

Here is the relevant piece of the build log for the reference

Step 5 (build-unified-tree) failure: build (failure)
...
991.278 [87/11/7452] Creating library symlink lib/libMLIRTestToLLVMIRTranslation.so
991.278 [87/10/7453] Creating library symlink lib/libMLIRTestIR.so
991.281 [86/10/7454] Creating library symlink lib/libMLIRTestPDLL.so
991.281 [86/9/7455] Creating library symlink lib/libMLIRTestTransforms.so
991.475 [86/8/7456] Linking CXX shared library lib/libMLIRTestPass.so.21.0git
991.485 [85/8/7457] Creating library symlink lib/libMLIRTestPass.so
991.686 [82/10/7458] Linking CXX executable bin/c-index-test
991.695 [82/9/7459] Linking CXX executable bin/mlir-query
991.785 [82/8/7460] Linking CXX executable bin/mlir-reduce
991.931 [82/7/7461] Linking CXX shared library lib/libFIRDialect.so.21.0git
FAILED: lib/libFIRDialect.so.21.0git 
: && /usr/local/bin/c++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-copy -Wno-string-conversion -Wno-ctad-maybe-unsupported -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3 -DNDEBUG  -Wl,-z,defs -Wl,-z,nodelete   -Wl,-rpath-link,/home/tcwg-buildbot/worker/flang-aarch64-sharedlibs/build/./lib  -Wl,--gc-sections -shared -Wl,-soname,libFIRDialect.so.21.0git -o lib/libFIRDialect.so.21.0git tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIRAttr.cpp.o tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIRDialect.cpp.o tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIRType.cpp.o tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FirAliasTagOpInterface.cpp.o tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FortranVariableInterface.cpp.o tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/Inliner.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib:/home/tcwg-buildbot/worker/flang-aarch64-sharedlibs/build/lib:"  lib/libCUFAttrs.so.21.0git  lib/libFIRDialectSupport.so.21.0git  lib/libLLVMAsmPrinter.so.21.0git  lib/libMLIRBuiltinToLLVMIRTranslation.so.21.0git  lib/libMLIROpenMPToLLVM.so.21.0git  lib/libMLIRLLVMToLLVMIRTranslation.so.21.0git  lib/libMLIRFuncToLLVM.so.21.0git  lib/libMLIRArithToLLVM.so.21.0git  lib/libMLIRArithAttrToLLVMConversion.so.21.0git  lib/libMLIRArithTransforms.so.21.0git  lib/libMLIRBufferizationTransforms.so.21.0git  lib/libMLIRBufferizationDialect.so.21.0git  lib/libMLIRSparseTensorDialect.so.21.0git  lib/libMLIRSCFDialect.so.21.0git  lib/libMLIRFuncTransforms.so.21.0git  lib/libMLIRShardingInterface.so.21.0git  lib/libMLIRMeshDialect.so.21.0git  lib/libMLIRVectorDialect.so.21.0git  lib/libMLIRTensorDialect.so.21.0git  lib/libMLIRParallelCombiningOpInterface.so.21.0git  lib/libMLIRMaskableOpInterface.so.21.0git  lib/libMLIRMaskingOpInterface.so.21.0git  lib/libMLIRVectorInterfaces.so.21.0git  lib/libMLIRControlFlowToLLVM.so.21.0git  lib/libMLIRControlFlowDialect.so.21.0git  lib/libMLIRMemRefToLLVM.so.21.0git  lib/libMLIRLLVMCommonConversion.so.21.0git  lib/libMLIRMemRefUtils.so.21.0git  lib/libMLIRAffineDialect.so.21.0git  lib/libMLIRMemRefDialect.so.21.0git  lib/libMLIRArithUtils.so.21.0git  lib/libMLIRComplexDialect.so.21.0git  lib/libMLIRArithDialect.so.21.0git  lib/libMLIRCastInterfaces.so.21.0git  lib/libMLIRInferIntRangeCommon.so.21.0git  lib/libMLIRShapedOpInterfaces.so.21.0git  lib/libMLIRDialect.so.21.0git  lib/libMLIRDialectUtils.so.21.0git  lib/libMLIROpenMPDialect.so.21.0git  lib/libMLIROpenACCMPCommon.so.21.0git  lib/libMLIRTargetLLVMIRExport.so.21.0git  lib/libMLIRDLTIDialect.so.21.0git  lib/libMLIRLLVMIRTransforms.so.21.0git  lib/libMLIRTransforms.so.21.0git  lib/libMLIRUBDialect.so.21.0git  lib/libMLIRRuntimeVerifiableOpInterface.so.21.0git  lib/libMLIRFuncDialect.so.21.0git  lib/libMLIRNVVMDialect.so.21.0git  lib/libMLIRTranslateLib.so.21.0git  lib/libMLIRParser.so.21.0git  lib/libMLIRBytecodeReader.so.21.0git  lib/libMLIRAsmParser.so.21.0git  lib/libMLIRTransformUtils.so.21.0git  lib/libMLIRSubsetOpInterface.so.21.0git  lib/libMLIRValueBoundsOpInterface.so.21.0git  lib/libMLIRDestinationStyleOpInterface.so.21.0git  lib/libMLIRRewrite.so.21.0git  lib/libMLIRRewritePDL.so.21.0git  lib/libMLIRPDLToPDLInterp.so.21.0git  lib/libMLIRPass.so.21.0git  lib/libMLIRAnalysis.so.21.0git  lib/libMLIRInferIntRangeInterface.so.21.0git  lib/libMLIRLoopLikeInterface.so.21.0git  lib/libMLIRPresburger.so.21.0git  lib/libMLIRViewLikeInterface.so.21.0git  lib/libMLIRPDLInterpDialect.so.21.0git  lib/libMLIRPDLDialect.so.21.0git  lib/libLLVMFrontendOpenMP.so.21.0git  lib/libLLVMTransformUtils.so.21.0git  lib/libMLIRLLVMDialect.so.21.0git  lib/libMLIRInferTypeOpInterface.so.21.0git  lib/libMLIRControlFlowInterfaces.so.21.0git  lib/libMLIRDataLayoutInterfaces.so.21.0git  lib/libMLIRFunctionInterfaces.so.21.0git  lib/libMLIRCallInterfaces.so.21.0git  lib/libMLIRMemorySlotInterfaces.so.21.0git  lib/libMLIRSideEffectInterfaces.so.21.0git  lib/libMLIRIR.so.21.0git  lib/libLLVMBitWriter.so.21.0git  lib/libLLVMAnalysis.so.21.0git  lib/libLLVMAsmParser.so.21.0git  lib/libLLVMBitReader.so.21.0git  lib/libMLIRSupport.so.21.0git  lib/libLLVMCore.so.21.0git  lib/libLLVMRemarks.so.21.0git  lib/libLLVMBinaryFormat.so.21.0git  lib/libLLVMTargetParser.so.21.0git  lib/libLLVMSupport.so.21.0git  -Wl,-rpath-link,/home/tcwg-buildbot/worker/flang-aarch64-sharedlibs/build/lib && :
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::CharBoxValue::dump() const':
FIROps.cpp:(.text._ZNK3fir12CharBoxValue4dumpEv[_ZNK3fir12CharBoxValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::CharBoxValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::PolymorphicValue::dump() const':
FIROps.cpp:(.text._ZNK3fir16PolymorphicValue4dumpEv[_ZNK3fir16PolymorphicValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::PolymorphicValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::ArrayBoxValue::dump() const':
FIROps.cpp:(.text._ZNK3fir13ArrayBoxValue4dumpEv[_ZNK3fir13ArrayBoxValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::ArrayBoxValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::CharArrayBoxValue::dump() const':
FIROps.cpp:(.text._ZNK3fir17CharArrayBoxValue4dumpEv[_ZNK3fir17CharArrayBoxValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::CharArrayBoxValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::ProcBoxValue::dump() const':
FIROps.cpp:(.text._ZNK3fir12ProcBoxValue4dumpEv[_ZNK3fir12ProcBoxValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::ProcBoxValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::BoxValue::dump() const':
FIROps.cpp:(.text._ZNK3fir8BoxValue4dumpEv[_ZNK3fir8BoxValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::BoxValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::MutableBoxValue::dump() const':
FIROps.cpp:(.text._ZNK3fir15MutableBoxValue4dumpEv[_ZNK3fir15MutableBoxValue4dumpEv]+0x20): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::MutableBoxValue const&)'
/usr/bin/ld: tools/flang/lib/Optimizer/Dialect/CMakeFiles/FIRDialect.dir/FIROps.cpp.o: in function `fir::ExtendedValue::dump() const':
FIROps.cpp:(.text._ZNK3fir13ExtendedValue4dumpEv[_ZNK3fir13ExtendedValue4dumpEv]+0x18): undefined reference to `fir::operator<<(llvm::raw_ostream&, fir::ExtendedValue const&)'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
992.049 [82/6/7462] Linking CXX shared library lib/libclangStaticAnalyzerCheckers.so.21.0git
992.274 [82/5/7463] Linking CXX executable bin/mlir-translate
992.754 [82/4/7464] Linking CXX shared library lib/libMLIRMlirOptMain.so.21.0git
992.788 [82/3/7465] Linking CXX executable bin/mlir-lsp-server
992.969 [82/2/7466] Linking CXX executable bin/mlir-opt
993.462 [82/1/7467] Linking CXX executable bin/mlir-runner
ninja: build stopped: subcommand failed.

[DavidSpickett]

I've reverted this because it broke shared library builds of flang: https://lab.llvm.org/buildbot/#/builders/80/builds/12361

That host is AArch64 but enabling shared libraries anywhere should reproduce it:

cmake -DLLVM_TARGETS_TO_BUILD=AArch64 -DBUILD_SHARED_LIBS=ON -DLLVM_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17 '-DLLVM_ENABLE_PROJECTS=llvm;clang;mlir;flang' '-DLLVM_ENABLE_RUNTIMES=flang-rt;openmp' -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON '-DLLVM_LIT_ARGS=-v -vv' -GNinja ../llvm-project/llvm

Probably you can find the library that contains the symbols it claims are missing, then add that to the dependencies of the thing it's trying to link in it's CMake config file.

[ashermancinelli]

@DavidSpickett Thank you, I'll look into this.

[ashermancinelli]

This was the extra include.

[jeanPerier]

Thanks Asher! I have a few post merge comments/questions.

[jeanPerier]

I think this could even be Pure since this is a "no-op" at runtime and can be safely hoisted from if/then/else and do loop regions.

[jeanPerier]

I believe fir.convert is used for PTR to INT conversions. Isn't this causing verifier issues when using LOC on a VOLATILE variable?

[ashermancinelli]

Yes, I just found this test in the gfortran test suite that shows this:

llvm-test-suite/Fortran/gfortran/regression/dec_loc_rval_2.f90

I'll correct it, thank you!

[jeanPerier]

Do you have a patch in your stack using this on the HLFIR operations (hlfir.assign, intrinsics, ....)?

[ashermancinelli]

I do - do you have concerns about them that I should look into?

[ashermancinelli]

Let me add some more tests in #134858.

[jeanPerier]

No specific concerns, I just wanted to make sure this was not forgotten and that the getEffects of HLFIR opertations would also be updated when relevant.
We do not have a lot of code motion at high level currently, but it will also be important to make sure high level operations referencing volatile memory are not moved without care.