[flang-commits] [flang] [flang][fir] fix ABI bug 116844 (PR #118121)
via flang-commits
flang-commits at lists.llvm.org
Fri Nov 29 11:08:18 PST 2024
https://github.com/jeanPerier created https://github.com/llvm/llvm-project/pull/118121
Fix issue #116844.
The issue came from a look-up on the func.func for the sret attribute when lowering fir.call with character arguments. This was broken because the func.func may or may not have been rewritten when dealing with the fir.call, but the lookup assumed it had not been rewritten yet. If the func.func was rewritten and the result moved to a sret argument, the call was lowered as if the character was meant to be the result, leading to bad call code and an assert.
It turns out that the whole logic is actually useless since fir.boxchar are never lowered as sret arguments, instead, lowering directly breaks the character result into the first two `fir.ref<>, i64` arguments. So, the sret case was actually never used, except in this bug.
Hence, instead of fixing the logic (probably by looking for argument attributes on the call itself), just remove this logic that brings unnecessary complexity.
Note: this is actually a very old bug that could be hit with complex result with the code below before #111969. This last patch exposed the issue by enabling a similar scenario with struct results.
```
function foo(c, i)
complex(16) :: foo
character(*) :: c
integer :: i
foo = (0., 0.)
end function
subroutine test(c)
character(*) :: c
complex(16) :: z, foo
z = foo(c, 1)
end subroutine
```
>From 8301c46755529092098e43a67b23dd0fc501e15a Mon Sep 17 00:00:00 2001
From: Jean Perier <jperier at nvidia.com>
Date: Fri, 29 Nov 2024 08:17:38 -0800
Subject: [PATCH] [flang][fir] fix ABI bug 116844
---
.../include/flang/Optimizer/CodeGen/Target.h | 10 +----
flang/lib/Optimizer/CodeGen/Target.cpp | 10 ++---
flang/lib/Optimizer/CodeGen/TargetRewrite.cpp | 38 ++++++-------------
flang/test/Fir/target-rewrite-boxchar.fir | 38 ++-----------------
flang/test/Fir/target.fir | 29 --------------
5 files changed, 21 insertions(+), 104 deletions(-)
diff --git a/flang/include/flang/Optimizer/CodeGen/Target.h b/flang/include/flang/Optimizer/CodeGen/Target.h
index 3b38583511927a..54bd8c9c457a1d 100644
--- a/flang/include/flang/Optimizer/CodeGen/Target.h
+++ b/flang/include/flang/Optimizer/CodeGen/Target.h
@@ -133,15 +133,7 @@ class CodeGenSpecifics {
/// Type representation of a `boxchar<n>` type argument when passed by value.
/// An argument value may need to be passed as a (safe) reference argument.
- ///
- /// A function that returns a `boxchar<n>` type value must already have
- /// converted that return value to a parameter decorated with the 'sret'
- /// Attribute (https://llvm.org/docs/LangRef.html#parameter-attributes).
- /// This requirement is in keeping with Fortran semantics, which require the
- /// caller to allocate the space for the return CHARACTER value and pass
- /// a pointer and the length of that space (a boxchar) to the called function.
- virtual Marshalling boxcharArgumentType(mlir::Type eleTy,
- bool sret = false) const = 0;
+ virtual Marshalling boxcharArgumentType(mlir::Type eleTy) const = 0;
// Compute ABI rules for an integer argument of the given mlir::IntegerType
// \p argTy. Note that this methods is supposed to be called for
diff --git a/flang/lib/Optimizer/CodeGen/Target.cpp b/flang/lib/Optimizer/CodeGen/Target.cpp
index 1a359ddd54dbe7..f7bffbf53c190e 100644
--- a/flang/lib/Optimizer/CodeGen/Target.cpp
+++ b/flang/lib/Optimizer/CodeGen/Target.cpp
@@ -80,17 +80,17 @@ struct GenericTarget : public CodeGenSpecifics {
mlir::TypeRange{ptrTy, idxTy});
}
- Marshalling boxcharArgumentType(mlir::Type eleTy, bool sret) const override {
+ Marshalling boxcharArgumentType(mlir::Type eleTy) const override {
CodeGenSpecifics::Marshalling marshal;
auto idxTy = mlir::IntegerType::get(eleTy.getContext(), S::defaultWidth);
auto ptrTy = fir::ReferenceType::get(eleTy);
marshal.emplace_back(ptrTy, AT{});
- // Return value arguments are grouped as a pair. Others are passed in a
- // split format with all pointers first (in the declared position) and all
- // LEN arguments appended after all of the dummy arguments.
+ // Characters are passed in a split format with all pointers first (in the
+ // declared position) and all LEN arguments appended after all of the dummy
+ // arguments.
// NB: Other conventions/ABIs can/should be supported via options.
marshal.emplace_back(idxTy, AT{/*alignment=*/0, /*byval=*/false,
- /*sret=*/sret, /*append=*/!sret});
+ /*sret=*/false, /*append=*/true});
return marshal;
}
diff --git a/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp b/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
index 04a3ea684642c8..4f5f1ff66718a2 100644
--- a/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
+++ b/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
@@ -403,7 +403,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
unsigned index = e.index();
llvm::TypeSwitch<mlir::Type>(ty)
.template Case<fir::BoxCharType>([&](fir::BoxCharType boxTy) {
- bool sret;
if constexpr (std::is_same_v<std::decay_t<A>, fir::CallOp>) {
if (noCharacterConversion) {
newInTyAndAttrs.push_back(
@@ -411,17 +410,14 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
newOpers.push_back(oper);
return;
}
- sret = callOp.getCallee() &&
- functionArgIsSRet(
- index, getModule().lookupSymbol<mlir::func::FuncOp>(
- *callOp.getCallee()));
} else {
- // TODO: dispatch case; how do we put arguments on a call?
- // We cannot put both an sret and the dispatch object first.
- sret = false;
- TODO(loc, "dispatch + sret not supported yet");
+ // TODO: dispatch case; it used to be a to-do because of sret,
+ // but is not tested and maybe should be removed. This pass is
+ // anyway ran after lowering fir.dispatch in flang, so maybe that
+ // should just be a requirement of the pass.
+ TODO(loc, "ABI of fir.dispatch with character arguments");
}
- auto m = specifics->boxcharArgumentType(boxTy.getEleTy(), sret);
+ auto m = specifics->boxcharArgumentType(boxTy.getEleTy());
auto unbox = rewriter->create<fir::UnboxCharOp>(
loc, std::get<mlir::Type>(m[0]), std::get<mlir::Type>(m[1]),
oper);
@@ -846,9 +842,8 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
// Convert a CHARACTER argument type. This can involve separating
// the pointer and the LEN into two arguments and moving the LEN
// argument to the end of the arg list.
- bool sret = functionArgIsSRet(index, func);
- for (auto e : llvm::enumerate(specifics->boxcharArgumentType(
- boxTy.getEleTy(), sret))) {
+ for (auto e : llvm::enumerate(
+ specifics->boxcharArgumentType(boxTy.getEleTy()))) {
auto &tup = e.value();
auto index = e.index();
auto attr = std::get<fir::CodeGenSpecifics::Attributes>(tup);
@@ -856,14 +851,9 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
if (attr.isAppend()) {
trailingTys.push_back(argTy);
} else {
- if (sret) {
- fixups.emplace_back(FixupTy::Codes::CharPair,
- newInTyAndAttrs.size(), index);
- } else {
- fixups.emplace_back(FixupTy::Codes::Trailing,
- newInTyAndAttrs.size(),
- trailingTys.size());
- }
+ fixups.emplace_back(FixupTy::Codes::Trailing,
+ newInTyAndAttrs.size(),
+ trailingTys.size());
newInTyAndAttrs.push_back(tup);
}
}
@@ -1112,12 +1102,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
(*fixup.finalizer)(func);
}
- inline bool functionArgIsSRet(unsigned index, mlir::func::FuncOp func) {
- if (auto attr = func.getArgAttrOfType<mlir::TypeAttr>(index, "llvm.sret"))
- return true;
- return false;
- }
-
template <typename Ty, typename FIXUPS>
void doReturn(mlir::func::FuncOp func, Ty &newResTys,
fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs,
diff --git a/flang/test/Fir/target-rewrite-boxchar.fir b/flang/test/Fir/target-rewrite-boxchar.fir
index b87cb35b46eb6c..681536cde6a05a 100644
--- a/flang/test/Fir/target-rewrite-boxchar.fir
+++ b/flang/test/Fir/target-rewrite-boxchar.fir
@@ -27,43 +27,13 @@ func.func @boxcharparams(%arg0 : !fir.boxchar<1>, %arg1 : !fir.boxchar<1>) -> i6
return %3 : i64
}
-// Test that we rewrite the signatures and bodies of functions that return a
-// boxchar.
-// INT32-LABEL: @boxcharsret
-// INT32-SAME: ([[ARG0:%[0-9A-Za-z]+]]: !fir.ref<!fir.char<1,?>> {llvm.sret = !fir.char<1,?>}, [[ARG1:%[0-9A-Za-z]+]]: i32, [[ARG2:%[0-9A-Za-z]+]]: !fir.ref<!fir.char<1,?>>, [[ARG3:%[0-9A-Za-z]+]]: i32)
-// INT64-LABEL: @boxcharsret
-// INT64-SAME: ([[ARG0:%[0-9A-Za-z]+]]: !fir.ref<!fir.char<1,?>> {llvm.sret = !fir.char<1,?>}, [[ARG1:%[0-9A-Za-z]+]]: i64, [[ARG2:%[0-9A-Za-z]+]]: !fir.ref<!fir.char<1,?>>, [[ARG3:%[0-9A-Za-z]+]]: i64)
-func.func @boxcharsret(%arg0 : !fir.boxchar<1> {llvm.sret = !fir.char<1,?>}, %arg1 : !fir.boxchar<1>) {
- // INT32-DAG: [[B0:%[0-9]+]] = fir.emboxchar [[ARG0]], [[ARG1]] : (!fir.ref<!fir.char<1,?>>, i32) -> !fir.boxchar<1>
- // INT32-DAG: [[B1:%[0-9]+]] = fir.emboxchar [[ARG2]], [[ARG3]] : (!fir.ref<!fir.char<1,?>>, i32) -> !fir.boxchar<1>
- // INT32-DAG: fir.unboxchar [[B0]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- // INT32-DAG: fir.unboxchar [[B1]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- // INT64-DAG: [[B0:%[0-9]+]] = fir.emboxchar [[ARG0]], [[ARG1]] : (!fir.ref<!fir.char<1,?>>, i64) -> !fir.boxchar<1>
- // INT64-DAG: [[B1:%[0-9]+]] = fir.emboxchar [[ARG2]], [[ARG3]] : (!fir.ref<!fir.char<1,?>>, i64) -> !fir.boxchar<1>
- // INT64-DAG: fir.unboxchar [[B0]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- // INT64-DAG: fir.unboxchar [[B1]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- %1:2 = fir.unboxchar %arg0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- %2:2 = fir.unboxchar %arg1 : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- %c0 = arith.constant 0 : index
- %c1 = arith.constant 1 : index
- %3 = fir.convert %1#1 : (i64) -> index
- %last = arith.subi %3, %c1 : index
- fir.do_loop %i = %c0 to %last step %c1 {
- %in_pos = fir.coordinate_of %2#0, %i : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
- %out_pos = fir.coordinate_of %1#0, %i : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
- %ch = fir.load %in_pos : !fir.ref<!fir.char<1>>
- fir.store %ch to %out_pos : !fir.ref<!fir.char<1>>
- }
- return
-}
-
-// Test that we rewrite the signatures of functions with a sret parameter and
+// Test that we rewrite the signatures of functions with several parameters.
// several other parameters.
// INT32-LABEL: @boxcharmultiple
-// INT32-SAME: ({{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>> {llvm.sret = !fir.char<1,?>}, {{%[0-9A-Za-z]+}}: i32, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: i32, {{%[0-9A-Za-z]+}}: i32)
+// INT32-SAME: ({{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: i32, {{%[0-9A-Za-z]+}}: i32, {{%[0-9A-Za-z]+}}: i32)
// INT64-LABEL: @boxcharmultiple
-// INT64-SAME: ({{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>> {llvm.sret = !fir.char<1,?>}, {{%[0-9A-Za-z]+}}: i64, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: i64, {{%[0-9A-Za-z]+}}: i64)
-func.func @boxcharmultiple(%arg0 : !fir.boxchar<1> {llvm.sret = !fir.char<1,?>}, %arg1 : !fir.boxchar<1>, %arg2 : !fir.boxchar<1>) {
+// INT64-SAME: ({{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: !fir.ref<!fir.char<1,?>>, {{%[0-9A-Za-z]+}}: i64, {{%[0-9A-Za-z]+}}: i64, {{%[0-9A-Za-z]+}}: i64)
+func.func @boxcharmultiple(%arg0 : !fir.boxchar<1>, %arg1 : !fir.boxchar<1>, %arg2 : !fir.boxchar<1>) {
return
}
diff --git a/flang/test/Fir/target.fir b/flang/test/Fir/target.fir
index 478d681a46e04f..d101db47c80513 100644
--- a/flang/test/Fir/target.fir
+++ b/flang/test/Fir/target.fir
@@ -110,32 +110,3 @@ func.func @char1lensum(%arg0 : !fir.boxchar<1>, %arg1 : !fir.boxchar<1>) -> i64
// X64: ret i64 %[[add]]
return %3 : i64
}
-
-// I32-LABEL: define void @char1copy(ptr nocapture sret(i8) %0, i32 %1, ptr nocapture %2, i32 %3)
-// I64-LABEL: define void @char1copy(ptr nocapture sret(i8) %0, i64 %1, ptr nocapture %2, i64 %3)
-// PPC-LABEL: define void @char1copy(ptr nocapture sret(i8) %0, i64 %1, ptr nocapture %2, i64 %3)
-func.func @char1copy(%arg0 : !fir.boxchar<1> {llvm.sret = !fir.char<1, ?>}, %arg1 : !fir.boxchar<1>) {
- // I32-DAG: %[[p0:.*]] = insertvalue { ptr, i32 } undef, ptr %2, 0
- // I32-DAG: = insertvalue { ptr, i32 } %[[p0]], i32 %3, 1
- // I32-DAG: %[[p1:.*]] = insertvalue { ptr, i32 } undef, ptr %0, 0
- // I32-DAG: = insertvalue { ptr, i32 } %[[p1]], i32 %1, 1
- // X64-DAG: %[[p0:.*]] = insertvalue { ptr, i64 } undef, ptr %2, 0
- // X64-DAG: = insertvalue { ptr, i64 } %[[p0]], i64 %3, 1
- // X64-DAG: %[[p1:.*]] = insertvalue { ptr, i64 } undef, ptr %0, 0
- // X64-DAG: = insertvalue { ptr, i64 } %[[p1]], i64 %1, 1
- %1:2 = fir.unboxchar %arg0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- %2:2 = fir.unboxchar %arg1 : (!fir.boxchar<1>) -> (!fir.ref<!fir.array<?x!fir.char<1>>>, i64)
- %c0 = arith.constant 0 : index
- %c1 = arith.constant 1 : index
- %3 = fir.convert %1#1 : (i64) -> index
- %last = arith.subi %3, %c1 : index
- fir.do_loop %i = %c0 to %last step %c1 {
- %in_pos = fir.coordinate_of %2#0, %i : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
- %out_pos = fir.coordinate_of %1#0, %i : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
- %ch = fir.load %in_pos : !fir.ref<!fir.char<1>>
- fir.store %ch to %out_pos : !fir.ref<!fir.char<1>>
- }
- // I32: ret void
- // X64: ret void
- return
-}
More information about the flang-commits
mailing list