Skip to content

Commit

Permalink
[flang][fir] fix ABI bug 116844 (llvm#118121)
Browse files Browse the repository at this point in the history 
Fix issue llvm#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.
  • Loading branch information
@jeanPerier
jeanPerier authored Dec 2, 2024
1 parent da23a83 commit cbb49d4
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 104 deletions.
10 changes: 1 addition & 9 deletions flang/include/flang/Optimizer/CodeGen/Target.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions flang/lib/Optimizer/CodeGen/Target.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
38 changes: 11 additions & 27 deletions flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,25 +403,21 @@ 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(
fir::CodeGenSpecifics::getTypeAndAttr(boxTy));
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);
Expand Down Expand Up @@ -846,24 +842,18 @@ 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);
auto argTy = std::get<mlir::Type>(tup);
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);
}
}
Expand Down Expand Up @@ -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,
Expand Down
38 changes: 4 additions & 34 deletions flang/test/Fir/target-rewrite-boxchar.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
29 changes: 0 additions & 29 deletions flang/test/Fir/target.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

0 comments on commit cbb49d4

Please sign in to comment.