[flang-commits] [flang] [flang] Hide strict volatility checks behind flag (PR #138183)
Asher Mancinelli via flang-commits
flang-commits at lists.llvm.org
Thu May 1 12:19:13 PDT 2025
https://github.com/ashermancinelli created https://github.com/llvm/llvm-project/pull/138183
Enabling volatility lowering by default revealed some issues in lowering and op verification.
For example, given volatile variable of a nested type, accessing structure members of a structure member would result in a volatility mismatch when the inner structure member is designated (and thus a verification error at compile time).
In other cases, I found correct codegen when the checks were disabled, also related to allocatable types and how we handle volatile references of boxes.
This hides the strict verification of fir and hlfir ops behind a flag so I can iteratively improve lowering of volatile variables without causing compile-time failures, keeping the strict verification on when running tests.
>From 9adde0b382811bbc39d00d181e3b05ba9141ff3e Mon Sep 17 00:00:00 2001
From: Asher Mancinelli <ashermancinelli at gmail.com>
Date: Thu, 1 May 2025 11:46:49 -0700
Subject: [PATCH] [flang] Hide strict volatility checks behind flag
Enabling volatility lowering by default revealed some issues in
lowering and op verification.
For example, given volatile variable of a nested type, accessing structure
members of a structure member would result in a volatility mismatch when
the inner structure member is designated (and thus a verification error
at compile time).
In other cases, I found correct codegen when the checks were disabled,
also related to allocatable types and how we handle volatile references
of boxes.
This hides the strict verification of fir and hlfir ops behind a flag
so I can iteratively improve lowering of volatile variables without
causing compile-time failures.
---
.../include/flang/Optimizer/Dialect/FIROps.h | 1 +
flang/lib/Optimizer/Dialect/FIROps.cpp | 27 +++++++++++++++----
flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp | 5 ++--
flang/test/Fir/invalid.fir | 4 +--
flang/test/Fir/volatile.fir | 2 +-
flang/test/Fir/volatile2.fir | 2 +-
flang/test/HLFIR/volatile.fir | 2 +-
flang/test/HLFIR/volatile1.fir | 2 +-
flang/test/HLFIR/volatile2.fir | 2 +-
flang/test/HLFIR/volatile3.fir | 2 +-
flang/test/HLFIR/volatile4.fir | 2 +-
flang/test/Lower/volatile-allocatable1.f90 | 17 ++++++++++++
flang/test/Lower/volatile-openmp.f90 | 2 +-
flang/test/Lower/volatile-string.f90 | 2 +-
flang/test/Lower/volatile1.f90 | 2 +-
flang/test/Lower/volatile2.f90 | 2 +-
flang/test/Lower/volatile3.f90 | 2 +-
flang/test/Lower/volatile4.f90 | 2 +-
18 files changed, 58 insertions(+), 22 deletions(-)
create mode 100644 flang/test/Lower/volatile-allocatable1.f90
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.h b/flang/include/flang/Optimizer/Dialect/FIROps.h
index 15bd512ea85af..1bed227afb50d 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.h
@@ -40,6 +40,7 @@ mlir::ParseResult parseSelector(mlir::OpAsmParser &parser,
mlir::OperationState &result,
mlir::OpAsmParser::UnresolvedOperand &selector,
mlir::Type &type);
+bool useStrictVolatileVerification();
static constexpr llvm::StringRef getNormalizedLowerBoundAttrName() {
return "normalized.lb";
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 8a24608336495..05ef69169bae5 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -33,11 +33,21 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Support/CommandLine.h"
namespace {
#include "flang/Optimizer/Dialect/CanonicalizationPatterns.inc"
} // namespace
+static llvm::cl::opt<bool> clUseStrictVolatileVerification(
+ "strict-fir-volatile-verifier", llvm::cl::init(false),
+ llvm::cl::desc(
+ "use stricter verifier for FIR operations with volatile types"));
+
+bool fir::useStrictVolatileVerification() {
+ return clUseStrictVolatileVerification;
+}
+
static void propagateAttributes(mlir::Operation *fromOp,
mlir::Operation *toOp) {
if (!fromOp || !toOp)
@@ -1535,11 +1545,14 @@ llvm::LogicalResult fir::ConvertOp::verify() {
// represent volatility.
const bool toLLVMPointer = mlir::isa<mlir::LLVM::LLVMPointerType>(outType);
const bool toInteger = fir::isa_integer(outType);
- if (fir::isa_volatile_type(inType) != fir::isa_volatile_type(outType) &&
- !toLLVMPointer && !toInteger)
- return emitOpError("cannot convert between volatile and non-volatile "
- "types, use fir.volatile_cast instead ")
- << inType << " / " << outType;
+ if (fir::useStrictVolatileVerification()) {
+ if (fir::isa_volatile_type(inType) != fir::isa_volatile_type(outType) &&
+ !toLLVMPointer && !toInteger) {
+ 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")
@@ -1841,6 +1854,10 @@ llvm::LogicalResult fir::TypeInfoOp::verify() {
static llvm::LogicalResult
verifyEmboxOpVolatilityInvariants(mlir::Type memrefType,
mlir::Type resultType) {
+
+ if (!fir::useStrictVolatileVerification())
+ return mlir::success();
+
mlir::Type boxElementType =
llvm::TypeSwitch<mlir::Type, mlir::Type>(resultType)
.Case<fir::BoxType, fir::ClassType>(
diff --git a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
index c5ed76753ea0c..eef1377f26961 100644
--- a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
+++ b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
@@ -423,8 +423,9 @@ llvm::LogicalResult hlfir::DesignateOp::verify() {
unsigned outputRank = 0;
mlir::Type outputElementType;
bool hasBoxComponent;
- if (fir::isa_volatile_type(memrefType) !=
- fir::isa_volatile_type(getResult().getType())) {
+ if (fir::useStrictVolatileVerification() &&
+ fir::isa_volatile_type(memrefType) !=
+ fir::isa_volatile_type(getResult().getType())) {
return emitOpError("volatility mismatch between memref and result type")
<< " memref type: " << memrefType
<< " result type: " << getResult().getType();
diff --git a/flang/test/Fir/invalid.fir b/flang/test/Fir/invalid.fir
index 447a6c68b4b0a..f9f5e267dd9bc 100644
--- a/flang/test/Fir/invalid.fir
+++ b/flang/test/Fir/invalid.fir
@@ -1,6 +1,6 @@
-// FIR ops diagnotic tests
-// RUN: fir-opt -split-input-file -verify-diagnostics %s
+
+// RUN: fir-opt -split-input-file -verify-diagnostics --strict-fir-volatile-verifier %s
// expected-error at +1{{custom op 'fir.string_lit' must have character type}}
%0 = fir.string_lit "Hello, World!"(13) : !fir.int<32>
diff --git a/flang/test/Fir/volatile.fir b/flang/test/Fir/volatile.fir
index 6b3d8709abdeb..9a7853083799f 100644
--- a/flang/test/Fir/volatile.fir
+++ b/flang/test/Fir/volatile.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" %s -o - | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" %s -o - | FileCheck %s
// CHECK: llvm.store volatile %{{.+}}, %{{.+}} : i32, !llvm.ptr
// CHECK: %{{.+}} = llvm.load volatile %{{.+}} : !llvm.ptr -> i32
func.func @foo() {
diff --git a/flang/test/Fir/volatile2.fir b/flang/test/Fir/volatile2.fir
index 82a8413d2fc02..d7c7351c361dd 100644
--- a/flang/test/Fir/volatile2.fir
+++ b/flang/test/Fir/volatile2.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt --fir-to-llvm-ir %s | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier --fir-to-llvm-ir %s | FileCheck %s
func.func @_QQmain() {
%0 = fir.alloca !fir.box<!fir.array<10xi32>, volatile>
%c1 = arith.constant 1 : index
diff --git a/flang/test/HLFIR/volatile.fir b/flang/test/HLFIR/volatile.fir
index 453413a93af44..6d43bf20a702b 100644
--- a/flang/test/HLFIR/volatile.fir
+++ b/flang/test/HLFIR/volatile.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt --convert-hlfir-to-fir %s -o - | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier --convert-hlfir-to-fir %s -o - | FileCheck %s
func.func @foo() {
%true = arith.constant true
diff --git a/flang/test/HLFIR/volatile1.fir b/flang/test/HLFIR/volatile1.fir
index 174acd77f9076..c6150fe72ed66 100644
--- a/flang/test/HLFIR/volatile1.fir
+++ b/flang/test/HLFIR/volatile1.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
func.func @_QQmain() attributes {fir.bindc_name = "p"} {
%0 = fir.address_of(@_QFEarr) : !fir.ref<!fir.array<10xi32>>
%c10 = arith.constant 10 : index
diff --git a/flang/test/HLFIR/volatile2.fir b/flang/test/HLFIR/volatile2.fir
index 86ac683adad3f..0501cfcc8e8ac 100644
--- a/flang/test/HLFIR/volatile2.fir
+++ b/flang/test/HLFIR/volatile2.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
func.func private @_QFPa() -> i32 attributes {fir.host_symbol = @_QQmain, llvm.linkage = #llvm.linkage<internal>} {
%0 = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFFaEa"}
%1 = fir.volatile_cast %0 : (!fir.ref<i32>) -> !fir.ref<i32, volatile>
diff --git a/flang/test/HLFIR/volatile3.fir b/flang/test/HLFIR/volatile3.fir
index 41e42916e8ee5..24ea4e4b6df97 100644
--- a/flang/test/HLFIR/volatile3.fir
+++ b/flang/test/HLFIR/volatile3.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
func.func @_QQmain() attributes {fir.bindc_name = "p"} {
%0 = fir.address_of(@_QFEarr) : !fir.ref<!fir.array<10xi32>>
%c10 = arith.constant 10 : index
diff --git a/flang/test/HLFIR/volatile4.fir b/flang/test/HLFIR/volatile4.fir
index cbf0aa31cb9f3..8980bcf932f81 100644
--- a/flang/test/HLFIR/volatile4.fir
+++ b/flang/test/HLFIR/volatile4.fir
@@ -1,4 +1,4 @@
-// RUN: fir-opt %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
+// RUN: fir-opt --strict-fir-volatile-verifier %s --bufferize-hlfir --convert-hlfir-to-fir | FileCheck %s
func.func @_QQmain() attributes {fir.bindc_name = "p"} {
%0 = fir.address_of(@_QFEarr) : !fir.ref<!fir.array<10xi32>>
%c10 = arith.constant 10 : index
diff --git a/flang/test/Lower/volatile-allocatable1.f90 b/flang/test/Lower/volatile-allocatable1.f90
new file mode 100644
index 0000000000000..a21359c3b4225
--- /dev/null
+++ b/flang/test/Lower/volatile-allocatable1.f90
@@ -0,0 +1,17 @@
+! RUN: bbc --strict-fir-volatile-verifier %s -o - | FileCheck %s
+
+! Requires correct propagation of volatility for allocatable nested types.
+! XFAIL: *
+
+function allocatable_udt()
+ type :: base_type
+ integer :: i = 42
+ end type
+ type, extends(base_type) :: ext_type
+ integer :: j = 100
+ end type
+ integer :: allocatable_udt
+ type(ext_type), allocatable, volatile :: v2(:,:)
+ allocate(v2(2,3))
+ allocatable_udt = v2(1,1)%i
+end function
diff --git a/flang/test/Lower/volatile-openmp.f90 b/flang/test/Lower/volatile-openmp.f90
index 3269af9618f10..6277cf942b8ec 100644
--- a/flang/test/Lower/volatile-openmp.f90
+++ b/flang/test/Lower/volatile-openmp.f90
@@ -1,4 +1,4 @@
-! RUN: bbc -fopenmp %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier -fopenmp %s -o - | FileCheck %s
type t
integer, pointer :: array(:)
end type
diff --git a/flang/test/Lower/volatile-string.f90 b/flang/test/Lower/volatile-string.f90
index 9173268880ace..88b21d7b245e9 100644
--- a/flang/test/Lower/volatile-string.f90
+++ b/flang/test/Lower/volatile-string.f90
@@ -1,4 +1,4 @@
-! RUN: bbc %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier %s -o - | FileCheck %s
program p
character(3), volatile :: string = 'foo'
character(3) :: nonvolatile_string
diff --git a/flang/test/Lower/volatile1.f90 b/flang/test/Lower/volatile1.f90
index 8447704619db0..385b9fa3bd1ad 100644
--- a/flang/test/Lower/volatile1.f90
+++ b/flang/test/Lower/volatile1.f90
@@ -1,4 +1,4 @@
-! RUN: bbc %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier %s -o - | FileCheck %s
program p
integer,volatile::i,arr(10)
diff --git a/flang/test/Lower/volatile2.f90 b/flang/test/Lower/volatile2.f90
index 4b7f185f24c41..defacf820bd54 100644
--- a/flang/test/Lower/volatile2.f90
+++ b/flang/test/Lower/volatile2.f90
@@ -1,4 +1,4 @@
-! RUN: bbc %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier %s -o - | FileCheck %s
program p
print*,a(),b(),c()
diff --git a/flang/test/Lower/volatile3.f90 b/flang/test/Lower/volatile3.f90
index dee6642e82593..8825f8f3afbcb 100644
--- a/flang/test/Lower/volatile3.f90
+++ b/flang/test/Lower/volatile3.f90
@@ -1,4 +1,4 @@
-! RUN: bbc %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier %s -o - | FileCheck %s
! Test that all combinations of volatile pointer and target are properly lowered -
! note that a volatile pointer implies that the target is volatile, even if not specified
diff --git a/flang/test/Lower/volatile4.f90 b/flang/test/Lower/volatile4.f90
index 42d7b68507b53..83ce2b8fdb25a 100644
--- a/flang/test/Lower/volatile4.f90
+++ b/flang/test/Lower/volatile4.f90
@@ -1,4 +1,4 @@
-! RUN: bbc %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier %s -o - | FileCheck %s
program p
integer,volatile::i,arr(10)
More information about the flang-commits
mailing list