[flang] Hide strict volatility checks behind flag #138183
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Asher Mancinelli (ashermancinelli) ChangesEnabling 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. Full diff: https://github.com/llvm/llvm-project/pull/138183.diff 18 Files Affected:
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@+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)
|
eugeneepshteyn
approved these changes
May 2, 2025
clementval
approved these changes
May 2, 2025
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
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.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
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.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
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.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.