Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise ABI-specific part of -preview=in #11828

Merged
merged 3 commits into from
Nov 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/dmd/frontend.h
Original file line number Diff line number Diff line change
Expand Up @@ -6541,7 +6541,7 @@ struct Target
TypeTuple* toArgTypes(Type* t);
bool isReturnOnStack(TypeFunction* tf, bool needsThis);
uint64_t parameterSize(const Loc& loc, Type* t);
void applyInRefParams(TypeFunction* tf);
bool preferPassByRef(Type* t);
private:
enum class TargetInfoKeys
{
Expand Down
8 changes: 4 additions & 4 deletions src/dmd/mtype.d
Original file line number Diff line number Diff line change
Expand Up @@ -6951,8 +6951,8 @@ extern (C++) final class Parameter : ASTNode
* Params:
* returnByRef = true if the function returns by ref
* p = Parameter to compare with
* previewIn = Whether `-previewIn` is being used, and thus if
* `in` means `scope`.
* previewIn = Whether `-preview=in` is being used, and thus if
* `in` means `scope [ref]`.
*
* Returns:
* true = `this` can be used in place of `p`
Expand All @@ -6972,8 +6972,8 @@ extern (C++) final class Parameter : ASTNode
otherSTC |= STC.scope_;
}

enum stc = STC.ref_ | STC.out_ | STC.lazy_;
if ((thisSTC & stc) != (otherSTC & stc))
const mask = STC.ref_ | STC.out_ | STC.lazy_ | (previewIn ? STC.in_ : 0);
if ((thisSTC & mask) != (otherSTC & mask))
return false;
return isCovariantScope(returnByRef, thisSTC, otherSTC);
}
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/mtype.h
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ class Parameter : public ASTNode
static size_t dim(Parameters *parameters);
static Parameter *getNth(Parameters *parameters, d_size_t nth);
const char *toChars() const;
bool isCovariant(bool returnByRef, const Parameter *p) const;
bool isCovariant(bool returnByRef, const Parameter *p, bool previewIn) const;
};

struct ParameterList
Expand Down
95 changes: 48 additions & 47 deletions src/dmd/target.d
Original file line number Diff line number Diff line change
Expand Up @@ -776,60 +776,61 @@ extern (C++) struct Target
}

/**
* Determine which `in` parameters needs to be passed by `ref`
*
* Called from `TypeFunction` semantic with the full function type.
* This routine must iterate over parameters, and may set `STC.ref_`
* for any parameter which already have `STC.in_`.
* This hook must never set `STC.ref_` if the parameter is not `STC.in_`,
* nor should it ever change anything else.
*
* This hook will not be called when `-preview=in` wasn't passed to the
* frontend, hence it needs not care about `params.previewIn`.
*
* Decides whether an `in` parameter of the specified POD type is to be
* passed by reference or by value. To be used with `-preview=in` only!
* Params:
* tf = Type of the function to inspect. The type will have its
* parameter types semantically resolved, however other attributes
* (return type, `@safe` / `nothrow`, etc...) must not be used.
* t = type of the `in` parameter, must be a POD
* Returns:
* `true` if the `in` parameter is to be passed by reference
*/
extern(C++) void applyInRefParams (TypeFunction tf)
extern (C++) bool preferPassByRef(Type t)
{
foreach (_idx, p; tf.parameterList)
const size = t.size();
if (global.params.is64bit)
{
// Ignore non-`in` or already-`ref` parameters
if ((p.storageClass & (STC.in_ | STC.ref_)) != STC.in_)
continue;

assert(p.type !is null);
// If it has a copy constructor / destructor / postblit,
// it is always by ref
if (p.type.needsDestruction() || p.type.needsCopyOrPostblit())
p.storageClass |= STC.ref_;
// If the type can't be copied, always by `ref`
else if (!p.type.isCopyable())
p.storageClass |= STC.ref_;
// The Win64 ABI requires x87 real to be passed by ref
else if (params.isWindows && params.is64bit &&
p.type.ty == Tfloat80)
p.storageClass |= STC.ref_;

// If it's a dynamic array, use the value type as it
// allows covariance between `in char[]` and `scope const(char)[]`
// The same reasoning applies to pointers and classes,
// but that is handled by the `(sz > 8)` below.
else if (p.type.ty == Tarray)
continue;
// Pass delegates by value to allow covariance
// Function pointers are a single pointers and handled below.
else if (p.type.ty == Tdelegate)
continue;
else
if (global.params.isWindows)
{
// Win64 special case: by-value for slices and delegates due to
// high number of usages in druntime/Phobos (compiled without
// -preview=in but supposed to link against -preview=in code)
const ty = t.toBasetype().ty;
if (ty == Tarray || ty == Tdelegate)
return false;

// If size is larger than 8 or not a power-of-2, the Win64 ABI
// would require a hidden reference anyway.
return size > 8
|| (size > 0 && (size & (size - 1)) != 0);
}
else // SysV x86_64 ABI
{
const sz = p.type.size();
if (params.is64bit ? (sz > 16) : (sz > 8))
p.storageClass |= STC.ref_;
// Prefer a ref if the POD cannot be passed in registers, i.e.,
// would be passed on the stack, *and* the size is > 16.
if (size <= 16)
return false;

TypeTuple getArgTypes()
{
import dmd.aggregate : Sizeok;
if (auto ts = t.toBasetype().isTypeStruct())
{
auto sd = ts.sym;
assert(sd.sizeok == Sizeok.done);
return sd.argTypes;
}
return toArgTypes(t);
}

TypeTuple argTypes = getArgTypes();
assert(argTypes !is null, "size == 0 should already be handled");
return argTypes.arguments.length == 0; // cannot be passed in registers
}
}
else // 32-bit x86 ABI
{
// Prefer a ref if the size is > 2 machine words.
return size > 8;
}
}

// this guarantees `getTargetInfo` and `allTargetInfos` remain in sync
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/target.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ struct Target
TypeTuple *toArgTypes(Type *t);
bool isReturnOnStack(TypeFunction *tf, bool needsThis);
d_uns64 parameterSize(const Loc& loc, Type *t);
void applyInRefParams(TypeFunction *tf);
bool preferPassByRef(Type *t);
Expression *getTargetInfo(const char* name, const Loc& loc);
};

Expand Down
14 changes: 9 additions & 5 deletions src/dmd/typesem.d
Original file line number Diff line number Diff line change
Expand Up @@ -1491,12 +1491,16 @@ extern(C++) Type typeSemantic(Type type, const ref Loc loc, Scope* sc)

// Remove redundant storage classes for type, they are already applied
fparam.storageClass &= ~(STC.TYPECTOR);
}

// Now that we're done processing the types of parameters,
// apply `STC.ref` where necessary
if (global.params.previewIn)
target.applyInRefParams(tf);
// -preview=in: add `ref` storage class to suited `in` params
if (global.params.previewIn && (fparam.storageClass & (STC.in_ | STC.ref_)) == STC.in_)
{
auto ts = t.baseElemOf().isTypeStruct();
const isPOD = !ts || ts.sym.isPOD();
if (!isPOD || target.preferPassByRef(t))
fparam.storageClass |= STC.ref_;
}
Comment on lines +1495 to +1502
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I'd knew we'd go this way earlier :)
Personally I don't mind, but it goes against @tsbockman 's review and what was discussed in the forum thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I've always mentioned that it should be based on the param type only. And the implementation proposals here and for LDC make no use the of param position, and I doubt Iain will make use of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only part that might be interesting is the relationship of the in parameter with other parameters, but that's something which is diagnosed much later, and not determinable from inspecting the function signature alone.

}

// Now that we completed semantic for the argument types,
// run semantic on their default values,
Expand Down
56 changes: 49 additions & 7 deletions test/compilable/previewin.d
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* REQUIRED_ARGS: -preview=dip1000 -preview=in
/* REQUIRED_ARGS: -preview=dip1000 -preview=in -mcpu=native
*/

import core.stdc.time;
Expand All @@ -24,14 +24,13 @@ struct FooBar
string toString() const
{
string result;
// Type is const
this.toString((in char[] buf) {
static assert(is(typeof(buf) == const(char[])));
result ~= buf;
});
// Type inference works
this.toString((buf) { result ~= buf; });
// Specifying the STC too
this.toString((in buf) { result ~= buf; });
// Some covariance
this.toString((const scope buf) { result ~= buf; });
this.toString((scope const(char)[] buf) { result ~= buf; });
this.toString((scope const(char[]) buf) { result ~= buf; });
return result;
}

Expand Down Expand Up @@ -64,3 +63,46 @@ void checkTemporary()
LError:
assert(0);
}


// Some ABI-specific tests:

version (Win64)
{
void checkReal(in real p)
{
// ref for x87 real, value for double-precision real
static assert(__traits(isRef, p) == (real.sizeof > 8));
}

struct RGB { ubyte r, g, b; }
void checkNonPowerOf2(in RGB p)
{
static assert(__traits(isRef, p));
}
}
else version (X86_64) // Posix x86_64
{
struct Empty {} // 1 dummy byte passed on the stack
void checkEmptyStruct(in Empty p)
{
static assert(!__traits(isRef, p));
}

static if (is(__vector(double[4])))
{
struct AvxVectorWrapper { __vector(double[4]) a; } // 256 bits
void checkAvxVector(in AvxVectorWrapper p)
{
static assert(!__traits(isRef, p));
}
}
}
else version (AArch64)
{
alias HVA = __vector(float[4])[4]; // can be passed in 4 vector registers
void checkHVA(in HVA p)
{
static assert(!__traits(isRef, p));
}
}
8 changes: 4 additions & 4 deletions test/fail_compilation/diagin.d
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
PERMUTE_ARGS: -preview=in
TEST_OUTPUT:
---
fail_compilation/diagin.d(14): Error: function `diagin.foo(in string)` is not callable using argument types `()`
fail_compilation/diagin.d(14): missing argument for parameter #1: `in string`
fail_compilation/diagin.d(14): Error: function `diagin.foo(in int)` is not callable using argument types `()`
fail_compilation/diagin.d(14): missing argument for parameter #1: `in int`
fail_compilation/diagin.d(16): Error: template `diagin.foo1` cannot deduce function from argument types `!()(bool[])`, candidates are:
fail_compilation/diagin.d(20): `foo1(T)(in T v, string)`
---
Expand All @@ -16,10 +16,10 @@ void main ()
foo1(lvalue);
}

void foo(in string) {}
void foo(in int) {}
void foo1(T)(in T v, string) {}

// Ensure that `in` has a unique mangling
static assert(foo.mangleof == `_D6diagin3fooFIAyaZv`);
static assert(foo.mangleof == `_D6diagin3fooFIiZv`);
static assert(foo1!int.mangleof == `_D6diagin__T4foo1TiZQiFNaNbNiNfIiAyaZv`);
static assert(foo1!char.mangleof == `_D6diagin__T4foo1TaZQiFNaNbNiNfIaAyaZv`);
60 changes: 17 additions & 43 deletions test/fail_compilation/previewin.d
Original file line number Diff line number Diff line change
Expand Up @@ -2,59 +2,33 @@
REQUIRED_ARGS: -preview=in -preview=dip1000
TEST_OUTPUT:
---
fail_compilation/previewin.d(3): Error: function `previewin.func1(void function(ulong[8]) dg)` is not callable using argument types `(void function(in ulong[8]))`
fail_compilation/previewin.d(3): cannot pass argument `& func_byRef` of type `void function(in ulong[8])` to parameter `void function(ulong[8]) dg`
fail_compilation/previewin.d(4): Error: function `previewin.func2(void function(ref ulong[8]) dg)` is not callable using argument types `(void function(in ulong[8]))`
fail_compilation/previewin.d(4): cannot pass argument `& func_byRef` of type `void function(in ulong[8])` to parameter `void function(ref ulong[8]) dg`
fail_compilation/previewin.d(7): Error: function `previewin.func4(void function(ref uint) dg)` is not callable using argument types `(void function(in uint))`
fail_compilation/previewin.d(7): cannot pass argument `& func_byValue` of type `void function(in uint)` to parameter `void function(ref uint) dg`
fail_compilation/previewin.d(41): Error: scope variable `arg` assigned to non-scope `myGlobal`
fail_compilation/previewin.d(42): Error: scope variable `arg` assigned to non-scope `myGlobal`
fail_compilation/previewin.d(43): Error: scope variable `arg` may not be returned
fail_compilation/previewin.d(44): Error: scope variable `arg` assigned to `escape` with longer lifetime
fail_compilation/previewin.d(48): Error: returning `arg` escapes a reference to parameter `arg`
fail_compilation/previewin.d(48): perhaps annotate the parameter with `return`
fail_compilation/previewin.d(4): Error: function `previewin.takeFunction(void function(in real) f)` is not callable using argument types `(void function(real x) pure nothrow @nogc @safe)`
fail_compilation/previewin.d(4): cannot pass argument `__lambda1` of type `void function(real x) pure nothrow @nogc @safe` to parameter `void function(in real) f`
fail_compilation/previewin.d(5): Error: function `previewin.takeFunction(void function(in real) f)` is not callable using argument types `(void function(const(real) x) pure nothrow @nogc @safe)`
fail_compilation/previewin.d(5): cannot pass argument `__lambda2` of type `void function(const(real) x) pure nothrow @nogc @safe` to parameter `void function(in real) f`
fail_compilation/previewin.d(6): Error: function `previewin.takeFunction(void function(in real) f)` is not callable using argument types `(void function(ref const(real) x) pure nothrow @nogc @safe)`
fail_compilation/previewin.d(6): cannot pass argument `__lambda3` of type `void function(ref const(real) x) pure nothrow @nogc @safe` to parameter `void function(in real) f`
fail_compilation/previewin.d(15): Error: scope variable `arg` assigned to non-scope `myGlobal`
fail_compilation/previewin.d(16): Error: scope variable `arg` assigned to non-scope `myGlobal`
fail_compilation/previewin.d(17): Error: scope variable `arg` may not be returned
fail_compilation/previewin.d(18): Error: scope variable `arg` assigned to `escape` with longer lifetime
fail_compilation/previewin.d(22): Error: returning `arg` escapes a reference to parameter `arg`
fail_compilation/previewin.d(22): perhaps annotate the parameter with `return`
---
*/

#line 1
void main ()
{
func1(&func_byRef); // No
func2(&func_byRef); // No
func3(&func_byRef); // Could be Yes, but currently No

func4(&func_byValue); // No
func5(&func_byValue); // Yes

func6(&func_byValue2); // Yes
func7(&func_byValue3); // Yes
// No covariance without explicit `in`
takeFunction((real x) {});
takeFunction((const scope real x) {});
takeFunction((const scope ref real x) {});

tryEscape("Hello World"); // Yes by `tryEscape` is NG
}

// Takes by `scope ref const`
void func_byRef(in ulong[8]) {}
// Takes by `scope const`
void func_byValue(in uint) {}

// Error: `ulong[8]` is passed by `ref`
void func1(void function(scope ulong[8]) dg) {}
// Error: Missing `scope` on a `ref`
void func2(void function(ref ulong[8]) dg) {}
// Works: `scope ref`
void func3(void function(scope const ref ulong[8]) dg) {}

// Error: `uint` is passed by value
void func4(void function(ref uint) dg) {}
// Works: By value `scope const`
void func5(void function(scope const uint) dg) {}

// This works for arrays:
void func_byValue2(in char[]) {}
void func6(void function(char[]) dg) {}
void func_byValue3(scope const(char)[]) {}
void func7(void function(in char[]) dg) {}
void takeFunction(void function(in real) f);

// Make sure things cannot be escaped (`scope` is applied)
const(char)[] myGlobal;
Expand Down
Loading