[flang][fir] fix ABI bug 116844 (#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.

flang/include/flang/Optimizer/CodeGen/Target.h

@@ -133,15 +133,7 @@ public:
 
   /// 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.
-  ///
+  virtual Marshalling boxcharArgumentType(mlir::Type eleTy) const = 0;
-  /// 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;
 
   // Compute ABI rules for an integer argument of the given mlir::IntegerType
   // \p argTy. Note that this methods is supposed to be called for

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
+    // Characters are passed in a split format with all pointers first (in the
-    // split format with all pointers first (in the declared position) and all
+    // declared position) and all LEN arguments appended after all of the dummy
-    // LEN arguments appended after all of the dummy arguments.
+    // 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;
   }
 

flang/lib/Optimizer/CodeGen/TargetRewrite.cpp

@@ -403,7 +403,6 @@ public:
       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 @@ public:
                 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?
+              // TODO: dispatch case; it used to be a to-do because of sret,
-              // We cannot put both an sret and the dispatch object first.
+              // but is not tested and maybe should be removed. This pass is
-              sret = false;
+              // anyway ran after lowering fir.dispatch in flang, so maybe that
-              TODO(loc, "dispatch + sret not supported yet");
+              // 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 @@ public:
               // 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(
-              for (auto e : llvm::enumerate(specifics->boxcharArgumentType(
+                       specifics->boxcharArgumentType(boxTy.getEleTy()))) {
-                       boxTy.getEleTy(), sret))) {
                 auto &tup = e.value();
                 auto index = e.index();
                 auto attr = std::get<fir::CodeGenSpecifics::Attributes>(tup);
@@ -856,14 +851,9 @@ public:
                 if (attr.isAppend()) {
                   trailingTys.push_back(argTy);
                 } else {
-                  if (sret) {
+                  fixups.emplace_back(FixupTy::Codes::Trailing,
-                    fixups.emplace_back(FixupTy::Codes::CharPair,
+                                      newInTyAndAttrs.size(),
-                                        newInTyAndAttrs.size(), index);
+                                      trailingTys.size());
-                  } else {
-                    fixups.emplace_back(FixupTy::Codes::Trailing,
-                                        newInTyAndAttrs.size(),
-                                        trailingTys.size());
-                  }
                   newInTyAndAttrs.push_back(tup);
                 }
               }
@@ -1112,12 +1102,6 @@ public:
         (*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,

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
+// Test that we rewrite the signatures of functions with several parameters.
-// 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
 // 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)
+// 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> {llvm.sret = !fir.char<1,?>}, %arg1 : !fir.boxchar<1>, %arg2 : !fir.boxchar<1>) {
+func.func @boxcharmultiple(%arg0 : !fir.boxchar<1>, %arg1 : !fir.boxchar<1>, %arg2 : !fir.boxchar<1>) {
   return
 }
 

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
-}