[flang][fir] fix ABI bug 116844

jeanPerier · jeanPerier · commit 8301c4675552 · 2024-11-29T08:17:38.000-08:00
diff --git 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
@@ -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
@@ -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);
@@ -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);
                 }
               }
@@ -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
@@ -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
@@ -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
-}
