[flang][fir] fix ABI bug 116844 #118121
Merged
[flang][fir] fix ABI bug 116844 #118121
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
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-codegen @llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) ChangesFix 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 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. Full diff: https://github.com/llvm/llvm-project/pull/118121.diff 5 Files Affected:
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
-}
|
|
Thanks for the quick turnaround for this fix! Much appreciated! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/11704 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/14140 Here is the relevant piece of the build log for the reference |
jeanPerier
added a commit
to jeanPerier/llvm-project
that referenced
this pull request
Dec 2, 2024
jeanPerier
added a commit
that referenced
this pull request
Dec 2, 2024
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.
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<>, i64arguments. 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.