Revise ABI-specific part of -preview=in

This is the promised follow-up on dlang#11000 and makes DMD exploit the
specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86).

It also almost perfectly matches the proposed LDC implementation in
ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic
arrays, but in that point the LDC and DMD Win64 ABI diverges in
general).

Author: kinke

src/dmd/frontend.h

@@ -6472,7 +6472,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);
     BEGIN_ENUM(TargetInfoKeys, TARGETINFOKEYS, targetinfokeys)
         ENUM_KEY(int32_t, cppRuntimeLibrary, 0, TargetInfoKeys, TARGETINFOKEYS, targetinfokeys, TIK)
         ENUM_KEY(int32_t, cppStd, 1, TargetInfoKeys, TARGETINFOKEYS, targetinfokeys, TIK)

src/dmd/target.d

@@ -772,60 +772,54 @@ 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 `global.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 (global.params.isWindows && global.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)
+            {
+                // 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 (global.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

src/dmd/target.h

@@ -108,7 +108,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);
 };
 

src/dmd/typesem.d

@@ -1484,12 +1484,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_;
+                }
+            }
 
             // Now that we completed semantic for the argument types,
             // run semantic on their default values,

test/compilable/previewin.d

@@ -1,4 +1,4 @@
-/* REQUIRED_ARGS: -preview=dip1000 -preview=in
+/* REQUIRED_ARGS: -preview=dip1000 -preview=in -mcpu=native
  */
 
 import core.stdc.time;
@@ -64,3 +64,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));
+    }
+}

test/runnable/previewin.d

@@ -13,11 +13,10 @@ void testWithAllAttributes() @safe pure nothrow @nogc
 
     // rvalues
     testin1(42);
-    testin2([42, ulong.max, ulong.min, 42UL]);
+    testin2((ulong[64]).init);
     testin3(ValueT(42));
     testin3(RefT(42));
-    testin4([ValueT(0), ValueT(1), ValueT(2), ValueT(3),
-             ValueT(4), ValueT(5), ValueT(6), ValueT(7)]);
+    testin4((ValueT[64]).init);
     testin4([RefT(42), RefT(84), RefT(126), RefT(4)]);
     testin5(NonCopyable(true));
     testin6(WithPostblit(true));
@@ -27,14 +26,14 @@ void testWithAllAttributes() @safe pure nothrow @nogc
     isTestOver = false;
 
     // lvalues
-    uint      a1;
-    ulong[4]  a2;
-    ValueT    a3;
-    ValueT[8] a4;
-    RefT      a5;
-    RefT[4]   a6;
+    uint       a1;
+    ulong[64]  a2;
+    ValueT     a3;
+    ValueT[64] a4;
+    RefT       a5;
+    RefT[4]    a6;
     NonCopyable  a7 = NonCopyable(true);
-    WithPostblit  a8;
+    WithPostblit a8;
     WithCopyCtor a9;
     WithDtor     a10 = WithDtor(&isTestOver);
 
@@ -118,7 +117,7 @@ void testForeach() @safe pure
 }
 
 struct ValueT { int value; }
-struct RefT   { ubyte[64] value; }
+struct RefT   { ulong[64] value; }
 
 struct NonCopyable
 {
@@ -155,25 +154,26 @@ struct WithDtor
 @safe pure nothrow @nogc:
 
 // By value
-void testin1(in uint p) {}
+void testin1(in uint p) { static assert(!__traits(isRef, p)); }
 // By ref because of size
-void testin2(in ulong[4] p) {}
+void testin2(in ulong[64] p) { static assert(__traits(isRef, p)); }
 // By value or ref depending on size
-void testin3(in ValueT p) {}
-void testin3(in RefT p) {}
+void testin3(in ValueT p) { static assert(!__traits(isRef, p)); }
+void testin3(in RefT p) { static assert(__traits(isRef, p)); }
 // By ref because of size
-void testin4(in ValueT[8] p) {}
-void testin4(in RefT[4] p) {}
+void testin4(in ValueT[64] p) { static assert(__traits(isRef, p)); }
+void testin4(in RefT[4] p) { static assert(__traits(isRef, p)); }
 
 // By ref because of non-copyability
-void testin5(in NonCopyable noncopy) {}
+void testin5(in NonCopyable noncopy) { static assert(__traits(isRef, noncopy)); }
 //  By ref because of postblit
-void testin6(in WithPostblit withposblit) {}
+void testin6(in WithPostblit withpostblit) { static assert(__traits(isRef, withpostblit)); }
 //  By ref because of copy ctor
-void testin7(in WithCopyCtor withcopy) {}
+void testin7(in WithCopyCtor withcopy) { static assert(__traits(isRef, withcopy)); }
 //  By ref because of dtor
 void testin8(in WithDtor withdtor, scope bool* isTestOver)
 {
+    static assert(__traits(isRef, withdtor));
     if (isTestOver)
         *isTestOver = true;
 }