From 5ae5fc1dbbbe00add8f2b9b28d8d318957cdee85 Mon Sep 17 00:00:00 2001 From: Kai Nacke Date: Sun, 14 Aug 2016 10:36:18 +0200 Subject: [PATCH 1/4] PPC/PPC64: Fix ABI errors. Issue #1652 seems to be caused by ABI errors in the bootstrap compiler. This PR updates the PPC/PPC64 ABI to conform to clang/gcc output. --- gen/abi-ppc.cpp | 72 +++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 23 deletions(-) diff --git a/gen/abi-ppc.cpp b/gen/abi-ppc.cpp index f213981..5fd245e 100644 --- a/gen/abi-ppc.cpp +++ b/gen/abi-ppc.cpp @@ -29,6 +29,8 @@ struct PPCTargetABI : TargetABI { ExplicitByvalRewrite byvalRewrite; + CompositeToArray32 compositeToArray32; + CompositeToArray64 compositeToArray64; IntegerRewrite integerRewrite; const bool Is64Bit; @@ -39,20 +41,49 @@ struct PPCTargetABI : TargetABI { return false; } - // Return structs and static arrays on the stack. The latter is needed - // because otherwise LLVM tries to actually return the array in a number - // of physical registers, which leads, depending on the target, to - // either horrendous codegen or backend crashes. + // FIXME Type *rt = tf->next->toBasetype(); - return (rt->ty == Tstruct || rt->ty == Tsarray); + if (tf->linkage == LINKd) + return rt->ty == Tsarray || rt->ty == Tstruct; + + // The ABI specifies that aggregates of size 8 bytes or less are + // returned in r3/r4 (ppc) or in r3 (ppc64). Looking at the IR + // generated by clang this seems not to be implemented. Regardless + // of size, the aggregate is always returned as sret. + return rt->ty == Tsarray || rt->ty == Tstruct; } bool passByVal(Type *t) override { - TY ty = t->toBasetype()->ty; - return ty == Tstruct || ty == Tsarray; + // On ppc, aggregates are always passed as an indirect value. + // On ppc64, they are always passed by value. However, clang + // used byval for type > 64 bytes. + t = t->toBasetype(); + return (t->ty == Tsarray || t->ty == Tstruct) && (!Is64Bit || t->size() > 64); } void rewriteFunctionType(TypeFunction *tf, IrFuncTy &fty) override { + // RETURN VALUE + Type *retTy = fty.ret->type->toBasetype(); + if (!fty.ret->byref) { + if (retTy->ty == Tstruct || retTy->ty == Tsarray) { + if (canRewriteAsInt(retTy, Is64Bit)) { + fty.ret->rewrite = &integerRewrite; + fty.ret->ltype = integerRewrite.type(fty.ret->type); + } else { + if (Is64Bit) { + fty.ret->rewrite = &compositeToArray64; + fty.ret->ltype = + compositeToArray64.type(fty.ret->type); + } else { + fty.ret->rewrite = &compositeToArray32; + fty.ret->ltype = + compositeToArray32.type(fty.ret->type); + } + } + } else if (retTy->isintegral()) + fty.ret->attrs.add(retTy->isunsigned() ? LLAttribute::ZExt + : LLAttribute::SExt); + } // EXPLICIT PARAMETERS for (auto arg : fty.args) { if (!arg->byref) { @@ -66,24 +97,19 @@ struct PPCTargetABI : TargetABI { if (ty->ty == Tstruct || ty->ty == Tsarray) { if (canRewriteAsInt(ty, Is64Bit)) { - if (!IntegerRewrite::isObsoleteFor(arg.ltype)) { - arg.rewrite = &integerRewrite; - arg.ltype = integerRewrite.type(arg.type); - } + arg.rewrite = &integerRewrite; + arg.ltype = integerRewrite.type(arg.type); } else { - // these types are passed byval: - // the caller allocates a copy and then passes a pointer to the copy - arg.rewrite = &byvalRewrite; - arg.ltype = byvalRewrite.type(arg.type); - - // the copy is treated as a local variable of the callee - // hence add the NoAlias and NoCapture attributes - arg.attrs.clear() - .add(LLAttribute::NoAlias) - .add(LLAttribute::NoCapture) - .addAlignment(byvalRewrite.alignment(arg.type)); + if (Is64Bit) { + arg.rewrite = &compositeToArray64; + arg.ltype = compositeToArray64.type(arg.type); + } else { + arg.rewrite = &compositeToArray32; + arg.ltype = compositeToArray32.type(arg.type); + } } - } + } else if (ty->isintegral()) + arg.attrs.add(ty->isunsigned() ? LLAttribute::ZExt : LLAttribute::SExt); } }; From 1449f9295bfac796d9a2e669c08b7016af4ac9ab Mon Sep 17 00:00:00 2001 From: Martin Date: Tue, 29 Nov 2016 20:29:01 +0100 Subject: [PATCH 2/4] PPC/PPC64: Reverse parameter order for extern(D) --- gen/abi-ppc.cpp | 44 ++++++++++++++------------------------------ 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/gen/abi-ppc.cpp b/gen/abi-ppc.cpp index 5fd245e..b50aa62 100644 --- a/gen/abi-ppc.cpp +++ b/gen/abi-ppc.cpp @@ -28,7 +28,6 @@ #include "gen/tollvm.h" struct PPCTargetABI : TargetABI { - ExplicitByvalRewrite byvalRewrite; CompositeToArray32 compositeToArray32; CompositeToArray64 compositeToArray64; IntegerRewrite integerRewrite; @@ -41,10 +40,7 @@ struct PPCTargetABI : TargetABI { return false; } - // FIXME Type *rt = tf->next->toBasetype(); - if (tf->linkage == LINKd) - return rt->ty == Tsarray || rt->ty == Tstruct; // The ABI specifies that aggregates of size 8 bytes or less are // returned in r3/r4 (ppc) or in r3 (ppc64). Looking at the IR @@ -58,38 +54,27 @@ struct PPCTargetABI : TargetABI { // On ppc64, they are always passed by value. However, clang // used byval for type > 64 bytes. t = t->toBasetype(); - return (t->ty == Tsarray || t->ty == Tstruct) && (!Is64Bit || t->size() > 64); + return (t->ty == Tsarray || t->ty == Tstruct) && + (!Is64Bit || t->size() > 64); } void rewriteFunctionType(TypeFunction *tf, IrFuncTy &fty) override { - // RETURN VALUE - Type *retTy = fty.ret->type->toBasetype(); + // return value if (!fty.ret->byref) { - if (retTy->ty == Tstruct || retTy->ty == Tsarray) { - if (canRewriteAsInt(retTy, Is64Bit)) { - fty.ret->rewrite = &integerRewrite; - fty.ret->ltype = integerRewrite.type(fty.ret->type); - } else { - if (Is64Bit) { - fty.ret->rewrite = &compositeToArray64; - fty.ret->ltype = - compositeToArray64.type(fty.ret->type); - } else { - fty.ret->rewrite = &compositeToArray32; - fty.ret->ltype = - compositeToArray32.type(fty.ret->type); - } - } - } else if (retTy->isintegral()) - fty.ret->attrs.add(retTy->isunsigned() ? LLAttribute::ZExt - : LLAttribute::SExt); + rewriteArgument(fty, *fty.ret); } - // EXPLICIT PARAMETERS + + // explicit parameters for (auto arg : fty.args) { if (!arg->byref) { rewriteArgument(fty, *arg); } } + + // extern(D): reverse parameter order for non variadics, for DMD-compliance + if (tf->linkage == LINKd && tf->varargs != 1 && fty.args.size() > 1) { + fty.reverseParams = true; + } } void rewriteArgument(IrFuncTy &fty, IrFuncTyArg &arg) override { @@ -108,12 +93,11 @@ struct PPCTargetABI : TargetABI { arg.ltype = compositeToArray32.type(arg.type); } } - } else if (ty->isintegral()) + } else if (ty->isintegral()) { arg.attrs.add(ty->isunsigned() ? LLAttribute::ZExt : LLAttribute::SExt); + } } }; // The public getter for abi.cpp -TargetABI *getPPCTargetABI(bool Is64Bit) { - return new PPCTargetABI(Is64Bit); -} +TargetABI *getPPCTargetABI(bool Is64Bit) { return new PPCTargetABI(Is64Bit); } From b34a03ddb6aba4e9e46dd5e726dd3856c00e9f2f Mon Sep 17 00:00:00 2001 From: Martin Date: Tue, 29 Nov 2016 21:29:14 +0100 Subject: [PATCH 3/4] Reverse parameter order for extern(D) for all other ABIs too --- gen/abi-aarch64.cpp | 5 +++++ gen/abi-mips64.cpp | 5 +++++ gen/abi-ppc64le.cpp | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/gen/abi-aarch64.cpp b/gen/abi-aarch64.cpp index 2952814..9cceb64 100644 --- a/gen/abi-aarch64.cpp +++ b/gen/abi-aarch64.cpp @@ -64,6 +64,11 @@ struct AArch64TargetABI : TargetABI { else if (passByVal(arg->type)) arg->attrs.remove(LLAttribute::ByVal); } + + // extern(D): reverse parameter order for non variadics, for DMD-compliance + if (tf->linkage == LINKd && tf->varargs != 1 && fty.args.size() > 1) { + fty.reverseParams = true; + } } void rewriteArgument(IrFuncTy &fty, IrFuncTyArg &arg) override { diff --git a/gen/abi-mips64.cpp b/gen/abi-mips64.cpp index b5082af..7f013f0 100644 --- a/gen/abi-mips64.cpp +++ b/gen/abi-mips64.cpp @@ -49,6 +49,11 @@ struct MIPS64TargetABI : TargetABI { rewriteArgument(fty, *arg); } } + + // extern(D): reverse parameter order for non variadics, for DMD-compliance + if (tf->linkage == LINKd && tf->varargs != 1 && fty.args.size() > 1) { + fty.reverseParams = true; + } } void rewriteArgument(IrFuncTy &fty, IrFuncTyArg &arg) override { diff --git a/gen/abi-ppc64le.cpp b/gen/abi-ppc64le.cpp index 2e6c498..731d8a6 100644 --- a/gen/abi-ppc64le.cpp +++ b/gen/abi-ppc64le.cpp @@ -83,6 +83,11 @@ struct PPC64LETargetABI : TargetABI { rewriteArgument(fty, *arg); } } + + // extern(D): reverse parameter order for non variadics, for DMD-compliance + if (tf->linkage == LINKd && tf->varargs != 1 && fty.args.size() > 1) { + fty.reverseParams = true; + } } void rewriteArgument(IrFuncTy &fty, IrFuncTyArg &arg) override { From 17a2c8ace21725ac80986a421520c845d1014a7b Mon Sep 17 00:00:00 2001 From: Martin Date: Tue, 29 Nov 2016 22:07:48 +0100 Subject: [PATCH 4/4] Attempt to fix PPC64-LE, MIPS64 and ARM/AArch64 ABIs Most importantly, make sure sret is enforced for all non-POD structs as done for all other ABIs, and don't treat extern(D) any different from the C ABI wrt. return values anymore. Also reduce code duplication via common TargetABI patterns. --- gen/abi-aarch64.cpp | 10 +++------- gen/abi-arm.cpp | 7 ++----- gen/abi-mips64.cpp | 10 +++++++++- gen/abi-ppc64le.cpp | 38 ++++++++------------------------------ 4 files changed, 22 insertions(+), 43 deletions(-) diff --git a/gen/abi-aarch64.cpp b/gen/abi-aarch64.cpp index 9cceb64..219531d 100644 --- a/gen/abi-aarch64.cpp +++ b/gen/abi-aarch64.cpp @@ -29,12 +29,10 @@ struct AArch64TargetABI : TargetABI { Type *rt = tf->next->toBasetype(); - // FIXME - if (tf->linkage == LINKd) - return rt->ty == Tsarray || rt->ty == Tstruct; + if (!isPOD(rt)) + return true; - return rt->ty == Tsarray || - (rt->ty == Tstruct && rt->size() > 16 && !isHFA((TypeStruct *)rt)); + return passByVal(rt); } bool passByVal(Type *t) override { @@ -61,8 +59,6 @@ struct AArch64TargetABI : TargetABI { for (auto arg : fty.args) { if (!arg->byref) rewriteArgument(fty, *arg); - else if (passByVal(arg->type)) - arg->attrs.remove(LLAttribute::ByVal); } // extern(D): reverse parameter order for non variadics, for DMD-compliance diff --git a/gen/abi-arm.cpp b/gen/abi-arm.cpp index d2442c0..a5dfa7e 100644 --- a/gen/abi-arm.cpp +++ b/gen/abi-arm.cpp @@ -33,11 +33,8 @@ struct ArmTargetABI : TargetABI { return false; Type *rt = tf->next->toBasetype(); - // For extern(D), always return structs by arg because of problem with - // non-POD structs (failure in std.algorithm.move when struct has a ctor). - // TODO: figure out what the problem is - if (tf->linkage == LINKd) - return rt->ty == Tsarray || rt->ty == Tstruct; + if (!isPOD(rt)) + return true; return rt->ty == Tsarray || (rt->ty == Tstruct && rt->size() > 4 && diff --git a/gen/abi-mips64.cpp b/gen/abi-mips64.cpp index 7f013f0..439a82f 100644 --- a/gen/abi-mips64.cpp +++ b/gen/abi-mips64.cpp @@ -30,11 +30,15 @@ struct MIPS64TargetABI : TargetABI { return false; } + Type *rt = tf->next->toBasetype(); + + if (!isPOD(rt)) + return true; + // Return structs and static arrays on the stack. The latter is needed // because otherwise LLVM tries to actually return the array in a number // of physical registers, which leads, depending on the target, to // either horrendous codegen or backend crashes. - Type *rt = tf->next->toBasetype(); return (rt->ty == Tstruct || rt->ty == Tsarray); } @@ -44,6 +48,10 @@ struct MIPS64TargetABI : TargetABI { } void rewriteFunctionType(TypeFunction *tf, IrFuncTy &fty) override { + if (!fty.ret->byref) { + rewriteArgument(fty, *fty.ret); + } + for (auto arg : fty.args) { if (!arg->byref) { rewriteArgument(fty, *arg); diff --git a/gen/abi-ppc64le.cpp b/gen/abi-ppc64le.cpp index 731d8a6..d0aa938 100644 --- a/gen/abi-ppc64le.cpp +++ b/gen/abi-ppc64le.cpp @@ -35,18 +35,10 @@ struct PPC64LETargetABI : TargetABI { Type *rt = tf->next->toBasetype(); - // FIXME: The return value of this function translates - // to RETstack or RETregs in function retStyle(), which - // directly influences if NRVO is possible or not - // (false -> RETregs -> nrvo_can = false). Depending on - // NRVO, the postblit constructor is called or not. - // Thus using the rules of the C ABI here (as mandated by - // the D specification) leads to crashes. - if (tf->linkage == LINKd) - return rt->ty == Tsarray || rt->ty == Tstruct; + if (!isPOD(rt)) + return true; - return rt->ty == Tsarray || (rt->ty == Tstruct && rt->size() > 16 && - !isHFA((TypeStruct *)rt, nullptr, 8)); + return passByVal(rt); } bool passByVal(Type *t) override { @@ -56,28 +48,13 @@ struct PPC64LETargetABI : TargetABI { } void rewriteFunctionType(TypeFunction *tf, IrFuncTy &fty) override { - // RETURN VALUE + // return value Type *retTy = fty.ret->type->toBasetype(); if (!fty.ret->byref) { - if (retTy->ty == Tstruct || retTy->ty == Tsarray) { - if (retTy->ty == Tstruct && - isHFA((TypeStruct *)retTy, &fty.ret->ltype, 8)) { - fty.ret->rewrite = &hfaToArray; - fty.ret->ltype = hfaToArray.type(fty.ret->type); - } else if (canRewriteAsInt(retTy, true)) { - fty.ret->rewrite = &integerRewrite; - fty.ret->ltype = integerRewrite.type(fty.ret->type); - } else { - fty.ret->rewrite = &compositeToArray64; - fty.ret->ltype = - compositeToArray64.type(fty.ret->type); - } - } else if (retTy->isintegral()) - fty.ret->attrs.add(retTy->isunsigned() ? LLAttribute::ZExt - : LLAttribute::SExt); + rewriteArgument(fty, *fty.ret); } - // EXPLICIT PARAMETERS + // explicit parameters for (auto arg : fty.args) { if (!arg->byref) { rewriteArgument(fty, *arg); @@ -103,8 +80,9 @@ struct PPC64LETargetABI : TargetABI { arg.rewrite = &compositeToArray64; arg.ltype = compositeToArray64.type(arg.type); } - } else if (ty->isintegral()) + } else if (ty->isintegral()) { arg.attrs.add(ty->isunsigned() ? LLAttribute::ZExt : LLAttribute::SExt); + } } };