Revert "[flang] correctly deal with bind(c) derived type result ABI" #111858
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-fir-hlfir @llvm/pr-subscribers-flang-codegen Author: None (jeanPerier) ChangesReverts llvm/llvm-project#111678 Causes ARM failure in test suite. TYPE(C_PTR) result should not regress even if struct ABI no implemented for the target. Patch is 32.02 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/111858.diff 7 Files Affected:
diff --git a/flang/include/flang/Optimizer/CodeGen/Target.h b/flang/include/flang/Optimizer/CodeGen/Target.h
index 3b38583511927a..a7161152a5c323 100644
--- a/flang/include/flang/Optimizer/CodeGen/Target.h
+++ b/flang/include/flang/Optimizer/CodeGen/Target.h
@@ -126,11 +126,6 @@ class CodeGenSpecifics {
structArgumentType(mlir::Location loc, fir::RecordType recTy,
const Marshalling &previousArguments) const = 0;
- /// Type representation of a `fir.type<T>` type argument when returned by
- /// value. Such value may need to be converted to a hidden reference argument.
- virtual Marshalling structReturnType(mlir::Location loc,
- fir::RecordType eleTy) const = 0;
-
/// 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.
///
diff --git a/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h b/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
index fb7b1d16f62f3a..cdbefdb2341485 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
@@ -177,27 +177,6 @@ inline mlir::NamedAttribute getAdaptToByRefAttr(Builder &builder) {
}
bool isDummyArgument(mlir::Value v);
-
-template <fir::FortranProcedureFlagsEnum Flag>
-inline bool hasProcedureAttr(fir::FortranProcedureFlagsEnumAttr flags) {
- return flags && bitEnumContainsAny(flags.getValue(), Flag);
-}
-
-template <fir::FortranProcedureFlagsEnum Flag>
-inline bool hasProcedureAttr(mlir::Operation *op) {
- if (auto firCallOp = mlir::dyn_cast<fir::CallOp>(op))
- return hasProcedureAttr<Flag>(firCallOp.getProcedureAttrsAttr());
- if (auto firCallOp = mlir::dyn_cast<fir::DispatchOp>(op))
- return hasProcedureAttr<Flag>(firCallOp.getProcedureAttrsAttr());
- return hasProcedureAttr<Flag>(
- op->getAttrOfType<fir::FortranProcedureFlagsEnumAttr>(
- getFortranProcedureFlagsAttrName()));
-}
-
-inline bool hasBindcAttr(mlir::Operation *op) {
- return hasProcedureAttr<fir::FortranProcedureFlagsEnum::bind_c>(op);
-}
-
} // namespace fir
#endif // FORTRAN_OPTIMIZER_DIALECT_FIROPSSUPPORT_H
diff --git a/flang/lib/Optimizer/CodeGen/Target.cpp b/flang/lib/Optimizer/CodeGen/Target.cpp
index 6c148dffb0e55a..a12b59413f4456 100644
--- a/flang/lib/Optimizer/CodeGen/Target.cpp
+++ b/flang/lib/Optimizer/CodeGen/Target.cpp
@@ -100,11 +100,6 @@ struct GenericTarget : public CodeGenSpecifics {
TODO(loc, "passing VALUE BIND(C) derived type for this target");
}
- CodeGenSpecifics::Marshalling
- structReturnType(mlir::Location loc, fir::RecordType ty) const override {
- TODO(loc, "returning BIND(C) derived type for this target");
- }
-
CodeGenSpecifics::Marshalling
integerArgumentType(mlir::Location loc,
mlir::IntegerType argTy) const override {
@@ -538,8 +533,7 @@ struct TargetX86_64 : public GenericTarget<TargetX86_64> {
/// When \p recTy is a one field record type that can be passed
/// like the field on its own, returns the field type. Returns
/// a null type otherwise.
- mlir::Type passAsFieldIfOneFieldStruct(fir::RecordType recTy,
- bool allowComplex = false) const {
+ mlir::Type passAsFieldIfOneFieldStruct(fir::RecordType recTy) const {
auto typeList = recTy.getTypeList();
if (typeList.size() != 1)
return {};
@@ -547,8 +541,6 @@ struct TargetX86_64 : public GenericTarget<TargetX86_64> {
if (mlir::isa<mlir::FloatType, mlir::IntegerType, fir::LogicalType>(
fieldType))
return fieldType;
- if (allowComplex && mlir::isa<mlir::ComplexType>(fieldType))
- return fieldType;
if (mlir::isa<fir::CharacterType>(fieldType)) {
// Only CHARACTER(1) are expected in BIND(C) contexts, which is the only
// contexts where derived type may be passed in registers.
@@ -601,7 +593,7 @@ struct TargetX86_64 : public GenericTarget<TargetX86_64> {
postMerge(byteOffset, Lo, Hi);
if (Lo == ArgClass::Memory || Lo == ArgClass::X87 ||
Lo == ArgClass::ComplexX87)
- return passOnTheStack(loc, recTy, /*isResult=*/false);
+ return passOnTheStack(loc, recTy);
int neededIntRegisters = 0;
int neededSSERegisters = 0;
if (Lo == ArgClass::SSE)
@@ -617,7 +609,7 @@ struct TargetX86_64 : public GenericTarget<TargetX86_64> {
// all in registers or all on the stack).
if (!hasEnoughRegisters(loc, neededIntRegisters, neededSSERegisters,
previousArguments))
- return passOnTheStack(loc, recTy, /*isResult=*/false);
+ return passOnTheStack(loc, recTy);
if (auto fieldType = passAsFieldIfOneFieldStruct(recTy)) {
CodeGenSpecifics::Marshalling marshal;
@@ -649,57 +641,9 @@ struct TargetX86_64 : public GenericTarget<TargetX86_64> {
return marshal;
}
- CodeGenSpecifics::Marshalling
- structReturnType(mlir::Location loc, fir::RecordType recTy) const override {
- std::uint64_t byteOffset = 0;
- ArgClass Lo, Hi;
- Lo = Hi = ArgClass::NoClass;
- byteOffset = classifyStruct(loc, recTy, byteOffset, Lo, Hi);
- mlir::MLIRContext *context = recTy.getContext();
- postMerge(byteOffset, Lo, Hi);
- if (Lo == ArgClass::Memory)
- return passOnTheStack(loc, recTy, /*isResult=*/true);
-
- // Note that X87/ComplexX87 are passed in memory, but returned via %st0
- // %st1 registers. Here, they are returned as fp80 or {fp80, fp80} by
- // passAsFieldIfOneFieldStruct, and LLVM will use the expected registers.
-
- // Note that {_Complex long double} is not 100% clear from an ABI
- // perspective because the aggregate post merger rules say it should be
- // passed in memory because it is bigger than 2 eight bytes. This has the
- // funny effect of
- // {_Complex long double} return to be dealt with differently than
- // _Complex long double.
-
- if (auto fieldType =
- passAsFieldIfOneFieldStruct(recTy, /*allowComplex=*/true)) {
- if (auto complexType = mlir::dyn_cast<mlir::ComplexType>(fieldType))
- return complexReturnType(loc, complexType.getElementType());
- CodeGenSpecifics::Marshalling marshal;
- marshal.emplace_back(fieldType, AT{});
- return marshal;
- }
-
- if (Hi == ArgClass::NoClass || Hi == ArgClass::SSEUp) {
- // Return a single integer or floating point argument.
- mlir::Type lowType = pickLLVMArgType(loc, context, Lo, byteOffset);
- CodeGenSpecifics::Marshalling marshal;
- marshal.emplace_back(lowType, AT{});
- return marshal;
- }
- // Will be returned in two different registers. Generate {lowTy, HiTy} for
- // the LLVM IR result type.
- CodeGenSpecifics::Marshalling marshal;
- mlir::Type lowType = pickLLVMArgType(loc, context, Lo, 8u);
- mlir::Type hiType = pickLLVMArgType(loc, context, Hi, byteOffset - 8u);
- marshal.emplace_back(mlir::TupleType::get(context, {lowType, hiType}),
- AT{});
- return marshal;
- }
-
/// Marshal an argument that must be passed on the stack.
- CodeGenSpecifics::Marshalling
- passOnTheStack(mlir::Location loc, mlir::Type ty, bool isResult) const {
+ CodeGenSpecifics::Marshalling passOnTheStack(mlir::Location loc,
+ mlir::Type ty) const {
CodeGenSpecifics::Marshalling marshal;
auto sizeAndAlign =
fir::getTypeSizeAndAlignmentOrCrash(loc, ty, getDataLayout(), kindMap);
@@ -707,7 +651,7 @@ struct TargetX86_64 : public GenericTarget<TargetX86_64> {
unsigned short align =
std::max(sizeAndAlign.second, static_cast<unsigned short>(8));
marshal.emplace_back(fir::ReferenceType::get(ty),
- AT{align, /*byval=*/!isResult, /*sret=*/isResult});
+ AT{align, /*byval=*/true, /*sret=*/false});
return marshal;
}
};
diff --git a/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp b/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
index 04a3ea684642c8..fd56fd6bf50f44 100644
--- a/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
+++ b/flang/lib/Optimizer/CodeGen/TargetRewrite.cpp
@@ -142,16 +142,20 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
mlir::ModuleOp getModule() { return getOperation(); }
- template <typename Ty, typename Callback>
+ template <typename A, typename B, typename C>
std::optional<std::function<mlir::Value(mlir::Operation *)>>
- rewriteCallResultType(mlir::Location loc, mlir::Type originalResTy,
- Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs,
- Callback &newOpers, mlir::Value &savedStackPtr,
- fir::CodeGenSpecifics::Marshalling &m) {
- // Currently, targets mandate COMPLEX or STRUCT is a single aggregate or
- // packed scalar, including the sret case.
- assert(m.size() == 1 && "return type not supported on this target");
+ rewriteCallComplexResultType(
+ mlir::Location loc, A ty, B &newResTys,
+ fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs, C &newOpers,
+ mlir::Value &savedStackPtr) {
+ if (noComplexConversion) {
+ newResTys.push_back(ty);
+ return std::nullopt;
+ }
+ auto m = specifics->complexReturnType(loc, ty.getElementType());
+ // Currently targets mandate COMPLEX is a single aggregate or packed
+ // scalar, including the sret case.
+ assert(m.size() == 1 && "target of complex return not supported");
auto resTy = std::get<mlir::Type>(m[0]);
auto attr = std::get<fir::CodeGenSpecifics::Attributes>(m[0]);
if (attr.isSRet()) {
@@ -166,7 +170,7 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
newInTyAndAttrs.push_back(m[0]);
newOpers.push_back(stack);
return [=](mlir::Operation *) -> mlir::Value {
- auto memTy = fir::ReferenceType::get(originalResTy);
+ auto memTy = fir::ReferenceType::get(ty);
auto cast = rewriter->create<fir::ConvertOp>(loc, memTy, stack);
return rewriter->create<fir::LoadOp>(loc, cast);
};
@@ -176,41 +180,11 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
// We are going to generate an alloca, so save the stack pointer.
if (!savedStackPtr)
savedStackPtr = genStackSave(loc);
- return this->convertValueInMemory(loc, call->getResult(0), originalResTy,
+ return this->convertValueInMemory(loc, call->getResult(0), ty,
/*inputMayBeBigger=*/true);
};
}
- template <typename Ty, typename Callback>
- std::optional<std::function<mlir::Value(mlir::Operation *)>>
- rewriteCallComplexResultType(
- mlir::Location loc, mlir::ComplexType ty, Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs, Callback &newOpers,
- mlir::Value &savedStackPtr) {
- if (noComplexConversion) {
- newResTys.push_back(ty);
- return std::nullopt;
- }
- auto m = specifics->complexReturnType(loc, ty.getElementType());
- return rewriteCallResultType(loc, ty, newResTys, newInTyAndAttrs, newOpers,
- savedStackPtr, m);
- }
-
- template <typename Ty, typename Callback>
- std::optional<std::function<mlir::Value(mlir::Operation *)>>
- rewriteCallStructResultType(
- mlir::Location loc, fir::RecordType recTy, Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs, Callback &newOpers,
- mlir::Value &savedStackPtr) {
- if (noStructConversion) {
- newResTys.push_back(recTy);
- return std::nullopt;
- }
- auto m = specifics->structReturnType(loc, recTy);
- return rewriteCallResultType(loc, recTy, newResTys, newInTyAndAttrs,
- newOpers, savedStackPtr, m);
- }
-
void passArgumentOnStackOrWithNewType(
mlir::Location loc, fir::CodeGenSpecifics::TypeAndAttr newTypeAndAttr,
mlir::Type oldType, mlir::Value oper,
@@ -382,11 +356,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
newInTyAndAttrs, newOpers,
savedStackPtr);
})
- .template Case<fir::RecordType>([&](fir::RecordType recTy) {
- wrap = rewriteCallStructResultType(loc, recTy, newResTys,
- newInTyAndAttrs, newOpers,
- savedStackPtr);
- })
.Default([&](mlir::Type ty) { newResTys.push_back(ty); });
} else if (fnTy.getResults().size() > 1) {
TODO(loc, "multiple results not supported yet");
@@ -593,24 +562,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
}
}
- template <typename Ty>
- void
- lowerStructSignatureRes(mlir::Location loc, fir::RecordType recTy,
- Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs) {
- if (noComplexConversion) {
- newResTys.push_back(recTy);
- return;
- } else {
- for (auto &tup : specifics->structReturnType(loc, recTy)) {
- if (std::get<fir::CodeGenSpecifics::Attributes>(tup).isSRet())
- newInTyAndAttrs.push_back(tup);
- else
- newResTys.push_back(std::get<mlir::Type>(tup));
- }
- }
- }
-
void
lowerStructSignatureArg(mlir::Location loc, fir::RecordType recTy,
fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs) {
@@ -644,9 +595,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
.Case<mlir::ComplexType>([&](mlir::ComplexType ty) {
lowerComplexSignatureRes(loc, ty, newResTys, newInTyAndAttrs);
})
- .Case<fir::RecordType>([&](fir::RecordType ty) {
- lowerStructSignatureRes(loc, ty, newResTys, newInTyAndAttrs);
- })
.Default([&](mlir::Type ty) { newResTys.push_back(ty); });
}
llvm::SmallVector<mlir::Type> trailingInTys;
@@ -748,8 +696,7 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
for (auto ty : func.getResults())
if ((mlir::isa<fir::BoxCharType>(ty) && !noCharacterConversion) ||
(fir::isa_complex(ty) && !noComplexConversion) ||
- (mlir::isa<mlir::IntegerType>(ty) && hasCCallingConv) ||
- (mlir::isa<fir::RecordType>(ty) && !noStructConversion)) {
+ (mlir::isa<mlir::IntegerType>(ty) && hasCCallingConv)) {
LLVM_DEBUG(llvm::dbgs() << "rewrite " << signature << " for target\n");
return false;
}
@@ -823,9 +770,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
rewriter->getUnitAttr()));
newResTys.push_back(retTy);
})
- .Case<fir::RecordType>([&](fir::RecordType recTy) {
- doStructReturn(func, recTy, newResTys, newInTyAndAttrs, fixups);
- })
.Default([&](mlir::Type ty) { newResTys.push_back(ty); });
// Saved potential shift in argument. Handling of result can add arguments
@@ -1118,12 +1062,21 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
return false;
}
+ /// Convert a complex return value. This can involve converting the return
+ /// value to a "hidden" first argument or packing the complex into a wide
+ /// GPR.
template <typename Ty, typename FIXUPS>
- void doReturn(mlir::func::FuncOp func, Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs,
- FIXUPS &fixups, fir::CodeGenSpecifics::Marshalling &m) {
- assert(m.size() == 1 &&
- "expect result to be turned into single argument or result so far");
+ void doComplexReturn(mlir::func::FuncOp func, mlir::ComplexType cmplx,
+ Ty &newResTys,
+ fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs,
+ FIXUPS &fixups) {
+ if (noComplexConversion) {
+ newResTys.push_back(cmplx);
+ return;
+ }
+ auto m =
+ specifics->complexReturnType(func.getLoc(), cmplx.getElementType());
+ assert(m.size() == 1);
auto &tup = m[0];
auto attr = std::get<fir::CodeGenSpecifics::Attributes>(tup);
auto argTy = std::get<mlir::Type>(tup);
@@ -1164,36 +1117,6 @@ class TargetRewrite : public fir::impl::TargetRewritePassBase<TargetRewrite> {
newResTys.push_back(argTy);
}
- /// Convert a complex return value. This can involve converting the return
- /// value to a "hidden" first argument or packing the complex into a wide
- /// GPR.
- template <typename Ty, typename FIXUPS>
- void doComplexReturn(mlir::func::FuncOp func, mlir::ComplexType cmplx,
- Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs,
- FIXUPS &fixups) {
- if (noComplexConversion) {
- newResTys.push_back(cmplx);
- return;
- }
- auto m =
- specifics->complexReturnType(func.getLoc(), cmplx.getElementType());
- doReturn(func, newResTys, newInTyAndAttrs, fixups, m);
- }
-
- template <typename Ty, typename FIXUPS>
- void doStructReturn(mlir::func::FuncOp func, fir::RecordType recTy,
- Ty &newResTys,
- fir::CodeGenSpecifics::Marshalling &newInTyAndAttrs,
- FIXUPS &fixups) {
- if (noStructConversion) {
- newResTys.push_back(recTy);
- return;
- }
- auto m = specifics->structReturnType(func.getLoc(), recTy);
- doReturn(func, newResTys, newInTyAndAttrs, fixups, m);
- }
-
template <typename FIXUPS>
void
createFuncOpArgFixups(mlir::func::FuncOp func,
diff --git a/flang/lib/Optimizer/Transforms/AbstractResult.cpp b/flang/lib/Optimizer/Transforms/AbstractResult.cpp
index c0ec820d87ed44..7299ff80121e13 100644
--- a/flang/lib/Optimizer/Transforms/AbstractResult.cpp
+++ b/flang/lib/Optimizer/Transforms/AbstractResult.cpp
@@ -32,33 +32,6 @@ using namespace mlir;
namespace fir {
namespace {
-// Helper to only build the symbol table if needed because its build time is
-// linear on the number of symbols in the module.
-struct LazySymbolTable {
- LazySymbolTable(mlir::Operation *op)
- : module{op->getParentOfType<mlir::ModuleOp>()} {}
- void build() {
- if (table)
- return;
- table = std::make_unique<mlir::SymbolTable>(module);
- }
-
- template <typename T>
- T lookup(llvm::StringRef name) {
- build();
- return table->lookup<T>(name);
- }
-
-private:
- std::unique_ptr<mlir::SymbolTable> table;
- mlir::ModuleOp module;
-};
-
-bool hasScalarDerivedResult(mlir::FunctionType funTy) {
- return funTy.getNumResults() == 1 &&
- mlir::isa<fir::RecordType>(funTy.getResult(0));
-}
-
static mlir::Type getResultArgumentType(mlir::Type resultType,
bool shouldBoxResult) {
return llvm::TypeSwitch<mlir::Type, mlir::Type>(resultType)
@@ -217,14 +190,7 @@ class SaveResultOpConversion
llvm::LogicalResult
matchAndRewrite(fir::SaveResultOp op,
mlir::PatternRewriter &rewriter) const override {
- mlir::Operation *call = op.getValue().getDefiningOp();
- if (mlir::isa<fir::RecordType>(op.getValue().getType()) && call &&
- fir::hasBindcAttr(call)) {
- rewriter.replaceOpWithNewOp<fir::StoreOp>(op, op.getValue(),
- op.getMemref());
- } else {
- rewriter.eraseOp(op);
- }
+ rewriter.eraseOp(op);
return mlir::success();
}
};
@@ -334,12 +300,6 @@ class AbstractResultOpt
auto *context = &getContext();
// Convert function type itself if it has an abstract result.
auto funcTy = mlir::cast<mlir::FunctionType>(func.getFunctionType());
- // Scalar derived result of BIND(C) function must be returned according
- // to the C struct return ABI which is target dependent and implemented in
- // the target-rewrite pass.
- if (hasScalarDerivedResult(funcTy) &&
- fir::hasBindcAttr(func.getOperation()))
- return;
if (hasAbstractResult(funcTy)) {...
[truncated]
|
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
…lvm#111858) Reverts llvm#111678 Causes ARM failure in test suite. TYPE(C_PTR) result should not regress even if struct ABI no implemented for the target. https://lab.llvm.org/buildbot/#/builders/143/builds/2731 I need to revisit this.
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.
Reverts #111678
Causes ARM failure in test suite. TYPE(C_PTR) result should not regress even if struct ABI no implemented for the target.
https://lab.llvm.org/buildbot/#/builders/143/builds/2731
I need to revisit this.