From 5e4ee132d9c54db36812a85c888abb0f2a33028a Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sun, 14 Nov 2021 16:42:19 +0100 Subject: [PATCH] Don't reverse parameters order for extern(D) --- CHANGELOG.md | 1 + gen/abi-nvptx.cpp | 1 - gen/abi-spirv.cpp | 1 - gen/abi-x86-64.cpp | 20 +++++------------ gen/abi-x86.cpp | 30 +++++++++++++------------- gen/abi.cpp | 9 -------- gen/abi.h | 5 ----- gen/dibuilder.cpp | 10 +-------- gen/functions.cpp | 12 +---------- gen/tocall.cpp | 6 +----- gen/uda.cpp | 8 ++----- ir/irfuncty.cpp | 3 +-- ir/irfuncty.h | 3 --- runtime/druntime | 2 +- runtime/jit-rt/d/ldc/dynamic_compile.d | 7 +++--- runtime/phobos | 2 +- tests/codegen/asm_gcc_no_fp.d | 2 +- tests/codegen/attr_allocsize.d | 4 ++-- tests/codegen/attr_param.d | 2 +- tests/codegen/gh2131.d | 2 +- tests/debuginfo/args_cdb.d | 2 +- 21 files changed, 38 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b166c59b421..4517432faf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - On Linux, LDC doesn't default to the `ld.gold` linker anymore. The combination of LLVM 13 and older gold linkers can apparently cause problems. We recommend using LLD, e.g., via `-linker=lld` or by setting your default `/usr/bin/ld` symlink; it's significantly faster too. - `-linkonce-templates` is less aggressive by default now and IMHO production-ready. (#3924) - When linking manually (not via LDC) against *shared* druntime, it is now required to link the bundled `lib/ldc_rt.dso.o[bj]` object file into each binary. It replaces the previously Windows-specific `dso_windows.obj`. (#3850) +- Breaking `extern(D)` ABI change for all targets: formal parameters of non-variadic functions aren't reversed anymore, in line with the spec. For 32-bit x86, the *first* parameter is accordingly now potentially passed in EAX, not the last one. So non-variadic `extern(D)` functions with multiple explicit parameters will break if expecting parameters in specific registers or stack slots, e.g., naked DMD-style inline assembly. (#3873, ldc-developers/phobos@3d725fce8f0acb78bf6cb984a8462e81e8e1b715) #### Bug fixes - Linux: Make LTO work with LLD. (#3786, #3850) diff --git a/gen/abi-nvptx.cpp b/gen/abi-nvptx.cpp index 79febc7ddc5..686e31a4f69 100644 --- a/gen/abi-nvptx.cpp +++ b/gen/abi-nvptx.cpp @@ -27,7 +27,6 @@ struct NVPTXTargetABI : TargetABI { t = t->toBasetype(); return ((t->ty == TY::Tsarray || t->ty == TY::Tstruct) && t->size() > 64); } - bool reverseExplicitParams(TypeFunction *) override { return false; } void rewriteFunctionType(IrFuncTy &fty) override { for (auto arg : fty.args) { if (!arg->byref) diff --git a/gen/abi-spirv.cpp b/gen/abi-spirv.cpp index 1aa8ebda390..53db25eadfd 100644 --- a/gen/abi-spirv.cpp +++ b/gen/abi-spirv.cpp @@ -27,7 +27,6 @@ struct SPIRVTargetABI : TargetABI { t = t->toBasetype(); return ((t->ty == TY::Tsarray || t->ty == TY::Tstruct) && t->size() > 64); } - bool reverseExplicitParams(TypeFunction *) override { return false; } void rewriteFunctionType(IrFuncTy &fty) override { for (auto arg : fty.args) { if (!arg->byref) diff --git a/gen/abi-x86-64.cpp b/gen/abi-x86-64.cpp index 992f8a161c2..a00dc93b009 100644 --- a/gen/abi-x86-64.cpp +++ b/gen/abi-x86-64.cpp @@ -295,24 +295,14 @@ void X86_64TargetABI::rewriteFunctionType(IrFuncTy &fty) { Logger::println("x86-64 ABI: Transforming argument types"); LOG_SCOPE; - int begin = 0, end = fty.args.size(), step = 1; - if (fty.reverseParams) { - begin = end - 1; - end = -1; - step = -1; - } - for (int i = begin; i != end; i += step) { - IrFuncTyArg &arg = *fty.args[i]; - - if (arg.byref) { - if (!arg.isByVal() && regCount.int_regs > 0) { + for (IrFuncTyArg *arg : fty.args) { + if (arg->byref) { + if (!arg->isByVal() && regCount.int_regs > 0) { regCount.int_regs--; } - - continue; + } else { + rewriteArgument(fty, *arg, regCount); } - - rewriteArgument(fty, arg, regCount); } // regCount (fty.tag) is now in the state after all implicit & formal args, diff --git a/gen/abi-x86.cpp b/gen/abi-x86.cpp index 20ce0b3e086..38d0952b459 100644 --- a/gen/abi-x86.cpp +++ b/gen/abi-x86.cpp @@ -193,31 +193,31 @@ struct X86TargetABI : TargetABI { sret->attrs.addAttribute(LLAttribute::InReg); } - // ... otherwise try the last argument + // ... otherwise try the first argument else if (!fty.args.empty()) { - // The last parameter is passed in EAX rather than being pushed on the + // The first parameter is passed in EAX rather than being pushed on the // stack if the following conditions are met: // * It fits in EAX. // * It is not a 3 byte struct. // * It is not a floating point type. - IrFuncTyArg *last = fty.args.back(); - if (last->rewrite == &indirectByvalRewrite || - (last->byref && !last->isByVal())) { - Logger::println("Putting last (byref) parameter in register"); - last->attrs.addAttribute(LLAttribute::InReg); + IrFuncTyArg &first = *fty.args[0]; + if (first.rewrite == &indirectByvalRewrite || + (first.byref && !first.isByVal())) { + Logger::println("Putting first (byref) parameter in register"); + first.attrs.addAttribute(LLAttribute::InReg); } else { - Type *lastTy = last->type->toBasetype(); - auto sz = lastTy->size(); - if (!lastTy->isfloating() && (sz == 1 || sz == 2 || sz == 4)) { + Type *firstTy = first.type->toBasetype(); + auto sz = firstTy->size(); + if (!firstTy->isfloating() && (sz == 1 || sz == 2 || sz == 4)) { // rewrite aggregates as integers to make inreg work - if (lastTy->ty == TY::Tstruct || lastTy->ty == TY::Tsarray) { - integerRewrite.applyTo(*last); + if (firstTy->ty == TY::Tstruct || firstTy->ty == TY::Tsarray) { + integerRewrite.applyTo(first); // undo byval semantics applied via passByVal() returning true - last->byref = false; - last->attrs.clear(); + first.byref = false; + first.attrs.clear(); } - last->attrs.addAttribute(LLAttribute::InReg); + first.attrs.addAttribute(LLAttribute::InReg); } } } diff --git a/gen/abi.cpp b/gen/abi.cpp index de46b6c6e39..b10fc7abfe4 100644 --- a/gen/abi.cpp +++ b/gen/abi.cpp @@ -185,13 +185,6 @@ bool TargetABI::preferPassByRef(Type *t) { ////////////////////////////////////////////////////////////////////////////// -bool TargetABI::reverseExplicitParams(TypeFunction *tf) { - // Required by druntime for extern(D), except for `, ...`-style variadics. - return isExternD(tf) && tf->parameterList.length() > 1; -} - -////////////////////////////////////////////////////////////////////////////// - void TargetABI::rewriteVarargs(IrFuncTy &fty, std::vector &args) { for (auto arg : args) { @@ -311,8 +304,6 @@ struct IntrinsicABI : TargetABI { bool passByVal(TypeFunction *, Type *t) override { return false; } - bool reverseExplicitParams(TypeFunction *) override { return false; } - void rewriteArgument(IrFuncTy &fty, IrFuncTyArg &arg) override { Type *ty = arg.type->toBasetype(); if (ty->ty != TY::Tstruct) { diff --git a/gen/abi.h b/gen/abi.h index 524534de1c8..c1f69046a1d 100644 --- a/gen/abi.h +++ b/gen/abi.h @@ -147,11 +147,6 @@ struct TargetABI { /// argument. virtual bool passThisBeforeSret(TypeFunction *tf) { return false; } - /// Returns true if the explicit parameters order is to be reversed. - /// Defaults to true for non-variadic extern(D) functions as required by - /// druntime. - virtual bool reverseExplicitParams(TypeFunction *tf); - /// Called to give ABI the chance to rewrite the types virtual void rewriteFunctionType(IrFuncTy &fty) = 0; virtual void rewriteVarargs(IrFuncTy &fty, std::vector &args); diff --git a/gen/dibuilder.cpp b/gen/dibuilder.cpp index 7555275c304..2ec29ba817c 100644 --- a/gen/dibuilder.cpp +++ b/gen/dibuilder.cpp @@ -705,9 +705,6 @@ DIType DIBuilder::CreateAArrayType(TypeAArray *type) { //////////////////////////////////////////////////////////////////////////////// -// new calling convention constant being proposed as a Dwarf extension -const unsigned DW_CC_D_dmd = 0x43; - DISubroutineType DIBuilder::CreateFunctionType(Type *type) { TypeFunction *t = type->isTypeFunction(); assert(t); @@ -718,12 +715,7 @@ DISubroutineType DIBuilder::CreateFunctionType(Type *type) { LLMetadata *params = {CreateTypeDescription(retType)}; auto paramsArray = DBuilder.getOrCreateTypeArray(params); - // The calling convention has to be recorded to distinguish - // extern(D) functions from extern(C++) ones. - unsigned CC = - getIrType(t, true)->getIrFuncTy().reverseParams ? DW_CC_D_dmd : 0; - - return DBuilder.createSubroutineType(paramsArray, DIFlags::FlagZero, CC); + return DBuilder.createSubroutineType(paramsArray, DIFlags::FlagZero, 0); } DISubroutineType DIBuilder::CreateEmptyFunctionType() { diff --git a/gen/functions.cpp b/gen/functions.cpp index 111bd6da7d2..9aa4e545bb4 100644 --- a/gen/functions.cpp +++ b/gen/functions.cpp @@ -222,8 +222,6 @@ llvm::FunctionType *DtoFunctionType(Type *type, IrFuncTy &irFty, Type *thistype, ++nextLLArgIdx; } - newIrFty.reverseParams = abi->reverseExplicitParams(f); - // let the ABI rewrite the types as necessary abi->rewriteFunctionType(newIrFty); @@ -254,17 +252,11 @@ llvm::FunctionType *DtoFunctionType(Type *type, IrFuncTy &irFty, Type *thistype, std::swap(argtypes[0], argtypes[1]); } - const size_t firstExplicitArg = argtypes.size(); const size_t numExplicitLLArgs = irFty.args.size(); for (size_t i = 0; i < numExplicitLLArgs; i++) { argtypes.push_back(irFty.args[i]->ltype); } - // reverse params? - if (irFty.reverseParams && numExplicitLLArgs > 1) { - std::reverse(argtypes.begin() + firstExplicitArg, argtypes.end()); - } - irFty.funcType = LLFunctionType::get(irFty.ret->ltype, argtypes, isLLVMVariadic); @@ -791,9 +783,7 @@ void DtoDeclareFunction(FuncDeclaration *fdecl, const bool willDefine) { unsigned int k = 0; for (; iarg != func->arg_end(); ++iarg) { - size_t llExplicitIdx = irFty.reverseParams ? irFty.args.size() - k - 1 : k; - ++k; - IrFuncTyArg *arg = irFty.args[llExplicitIdx]; + IrFuncTyArg *arg = irFty.args[k++]; if (!fdecl->parameters || arg->parametersIdx >= fdecl->parameters->length) { iarg->setName("unnamed"); diff --git a/gen/tocall.cpp b/gen/tocall.cpp index 2b0e26a0318..4c71dfadece 100644 --- a/gen/tocall.cpp +++ b/gen/tocall.cpp @@ -155,8 +155,6 @@ static void addExplicitArguments(std::vector &args, AttrSet &attrs, args.resize(implicitLLArgCount + explicitLLArgCount, static_cast(nullptr)); - // Iterate the explicit arguments from left to right in the D source, - // which is the reverse of the LLVM order if irFty.reverseParams is true. size_t dArgIndex = 0; for (size_t i = 0; i < explicitLLArgCount; ++i, ++dArgIndex) { const bool isVararg = (i >= formalLLArgCount); @@ -199,9 +197,7 @@ static void addExplicitArguments(std::vector &args, AttrSet &attrs, llvm::Value *llVal = irFty.putArg(*irArg, dval, isLValueExp, dArgIndex == explicitDArgCount - 1); - const size_t llArgIdx = - implicitLLArgCount + - (irFty.reverseParams ? explicitLLArgCount - i - 1 : i); + const size_t llArgIdx = implicitLLArgCount + i; llvm::Type *const paramType = (isVararg ? nullptr : calleeType->getParamType(llArgIdx)); diff --git a/gen/uda.cpp b/gen/uda.cpp index 4f7e0e57580..e782f2347b9 100644 --- a/gen/uda.cpp +++ b/gen/uda.cpp @@ -191,12 +191,8 @@ void applyAttrAllocSize(StructLiteralExp *sle, IrFunction *irFunc) { unsigned offset = llvmNumParams - numUserParams; // Calculate the param indices for the function as defined in LLVM IR - auto llvmSizeIdx = - irFunc->irFty.reverseParams ? numUserParams - sizeArgIdx - 1 : sizeArgIdx; - auto llvmNumIdx = - irFunc->irFty.reverseParams ? numUserParams - numArgIdx - 1 : numArgIdx; - llvmSizeIdx += offset; - llvmNumIdx += offset; + const auto llvmSizeIdx = sizeArgIdx + offset; + const auto llvmNumIdx = numArgIdx + offset; llvm::AttrBuilder builder; if (numArgIdx >= 0) { diff --git a/ir/irfuncty.cpp b/ir/irfuncty.cpp index 8952670b83a..cdc55706819 100644 --- a/ir/irfuncty.cpp +++ b/ir/irfuncty.cpp @@ -143,8 +143,7 @@ AttrSet IrFuncTy::getParamAttrs(bool passThisBeforeSret) { // Set attributes on the explicit parameters. const size_t n = args.size(); for (size_t k = 0; k < n; k++) { - const size_t i = idx + (reverseParams ? (n - k - 1) : k); - newAttrs.addToParam(i, args[k]->attrs); + newAttrs.addToParam(idx + k, args[k]->attrs); } return newAttrs; diff --git a/ir/irfuncty.h b/ir/irfuncty.h index 4877da52b89..38f0ecf97aa 100644 --- a/ir/irfuncty.h +++ b/ir/irfuncty.h @@ -105,9 +105,6 @@ struct IrFuncTy { using ArgList = std::vector; ArgList args; - // range of normal parameters to reverse - bool reverseParams = false; - // reserved for ABI-specific data void *tag = nullptr; diff --git a/runtime/druntime b/runtime/druntime index 9c8cd6e677b..644a04b9bbb 160000 --- a/runtime/druntime +++ b/runtime/druntime @@ -1 +1 @@ -Subproject commit 9c8cd6e677b2dc94549b33ff4dce1edf3952301f +Subproject commit 644a04b9bbbfeca0d57d127c99e9027c19068bf1 diff --git a/runtime/jit-rt/d/ldc/dynamic_compile.d b/runtime/jit-rt/d/ldc/dynamic_compile.d index 21622767966..352498a1c19 100644 --- a/runtime/jit-rt/d/ldc/dynamic_compile.d +++ b/runtime/jit-rt/d/ldc/dynamic_compile.d @@ -578,11 +578,10 @@ struct BindPayload(OF, F, int[] Index, Args...) static if (InvalidIndex != ind) { { - const ii = ParametersCount - i - 1; // reverse params - desc[ii].data = &(argStore.args[ind]); - desc[ii].size = (argStore.args[ind]).sizeof; + desc[i].data = &(argStore.args[ind]); + desc[i].size = (argStore.args[ind]).sizeof; alias T = FuncParams[i]; - desc[ii].type = (isAggregateType!T || isDelegate!T || isStaticArray!T ? ParamType.Aggregate : ParamType.Simple); + desc[i].type = (isAggregateType!T || isDelegate!T || isStaticArray!T ? ParamType.Aggregate : ParamType.Simple); } } } diff --git a/runtime/phobos b/runtime/phobos index 30162ba06d3..3d725fce8f0 160000 --- a/runtime/phobos +++ b/runtime/phobos @@ -1 +1 @@ -Subproject commit 30162ba06d3eb1f81034009ce108867a8e776acd +Subproject commit 3d725fce8f0acb78bf6cb984a8462e81e8e1b715 diff --git a/tests/codegen/asm_gcc_no_fp.d b/tests/codegen/asm_gcc_no_fp.d index efaea974201..27c0b7dc92d 100644 --- a/tests/codegen/asm_gcc_no_fp.d +++ b/tests/codegen/asm_gcc_no_fp.d @@ -13,7 +13,7 @@ byte32 xor(byte32 a, byte32 b) // CHECK-NEXT: .cfi_startproc byte32 r; // CHECK-NEXT: #APP - // CHECK-NEXT: vxorps %ymm0, %ymm1, %ymm0 + // CHECK-NEXT: vxorps %ymm0, %ymm0, %ymm1 // CHECK-NEXT: #NO_APP asm { "vxorps %0, %1, %2" : "=v" (r) : "v" (a), "v" (b); } // CHECK-NEXT: retq diff --git a/tests/codegen/attr_allocsize.d b/tests/codegen/attr_allocsize.d index 8b98ee3a612..026d073217d 100644 --- a/tests/codegen/attr_allocsize.d +++ b/tests/codegen/attr_allocsize.d @@ -39,5 +39,5 @@ class A // CHECK-DAG: attributes #[[ATTR0]] ={{.*}} allocsize(1,0) // CHECK-DAG: attributes #[[ATTR1]] ={{.*}} allocsize(2) -// CHECK-DAG: attributes #[[ATTR2]] ={{.*}} allocsize(3,1) -// CHECK-DAG: attributes #[[ATTR3]] ={{.*}} allocsize(4,2) +// CHECK-DAG: attributes #[[ATTR2]] ={{.*}} allocsize(0,2) +// CHECK-DAG: attributes #[[ATTR3]] ={{.*}} allocsize(1,3) diff --git a/tests/codegen/attr_param.d b/tests/codegen/attr_param.d index 0bdffb887dd..0a9c2be3998 100644 --- a/tests/codegen/attr_param.d +++ b/tests/codegen/attr_param.d @@ -7,8 +7,8 @@ import ldc.attributes; void foo(@llvmAttr("noalias") void* p) {} // CHECK: define{{.*}} @{{.*}}3bar -// CHECK-SAME: [16 x float]*{{.*}} noalias dereferenceable(64) %kernel // CHECK-SAME: float*{{.*}} noalias %data_arg +// CHECK-SAME: [16 x float]*{{.*}} noalias dereferenceable(64) %kernel void bar(@restrict float* data, @restrict ref const float[16] kernel) {} // CHECK: define{{.*}} @{{.*}}14classReference diff --git a/tests/codegen/gh2131.d b/tests/codegen/gh2131.d index ae49ca6f82b..635874bc144 100644 --- a/tests/codegen/gh2131.d +++ b/tests/codegen/gh2131.d @@ -1,7 +1,7 @@ // RUN: %ldc -O3 -output-ll -of=%t.ll %s && FileCheck %s < %t.ll // CHECK: define {{.*}}zeroext {{.*}}@{{.*}}_D6gh21313foo -// CHECK-SAME: i1 zeroext %x_arg +// CHECK-SAME: i1 {{(inreg )?}}zeroext %x_arg bool foo(bool x, ref bool o) { // CHECK-NOT: and i8 diff --git a/tests/debuginfo/args_cdb.d b/tests/debuginfo/args_cdb.d index 4ff06be8845..8a5ca175306 100644 --- a/tests/debuginfo/args_cdb.d +++ b/tests/debuginfo/args_cdb.d @@ -79,7 +79,7 @@ int byValue(ubyte ub, ushort us, uint ui, ulong ul, // check arguments with indirections // CDB: ?? c -// CHECK: cdouble +// CHECK: > struct cdouble // CHECK-NEXT: re : 8 // CHECK-NEXT: im : 9