From 308f06cdba85d81f9f4d08e54922aa6245a5b55e Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 3 Jun 2020 01:33:18 +0200 Subject: [PATCH 1/3] Get rid of obsolete __ArrayEq lowering --- dmd/dcast.d | 23 -------------------- dmd/expressionsem.d | 52 ++++++++++++++++++++++++++------------------- dmd/id.d | 1 - dmd/mtype.h | 1 - dmd/opover.d | 51 +++++--------------------------------------- runtime/druntime | 2 +- 6 files changed, 36 insertions(+), 94 deletions(-) diff --git a/dmd/dcast.d b/dmd/dcast.d index b6d0ff7317b..3f9ff1319d1 100644 --- a/dmd/dcast.d +++ b/dmd/dcast.d @@ -3523,29 +3523,6 @@ void fix16997(Scope* sc, UnaExp ue) } } -/*********************************** - * See if both types are arrays that can be compared - * for equality. Return true if so. - * If they are arrays, but incompatible, issue error. - * This is to enable comparing things like an immutable - * array with a mutable one. - */ -extern (C++) bool arrayTypeCompatible(Loc loc, Type t1, Type t2) -{ - t1 = t1.toBasetype().merge2(); - t2 = t2.toBasetype().merge2(); - - if ((t1.ty == Tarray || t1.ty == Tsarray || t1.ty == Tpointer) && (t2.ty == Tarray || t2.ty == Tsarray || t2.ty == Tpointer)) - { - if (t1.nextOf().implicitConvTo(t2.nextOf()) < MATCH.constant && t2.nextOf().implicitConvTo(t1.nextOf()) < MATCH.constant && (t1.nextOf().ty != Tvoid && t2.nextOf().ty != Tvoid)) - { - error(loc, "array equality comparison type mismatch, `%s` vs `%s`", t1.toChars(), t2.toChars()); - } - return true; - } - return false; -} - /*********************************** * See if both types are arrays that can be compared * for equality without any casting. Return true if so. diff --git a/dmd/expressionsem.d b/dmd/expressionsem.d index 0c4f5a08506..6d00c3376f5 100644 --- a/dmd/expressionsem.d +++ b/dmd/expressionsem.d @@ -10973,7 +10973,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor Type t1 = exp.e1.type.toBasetype(); Type t2 = exp.e2.type.toBasetype(); - bool needsDirectEq(Type t1, Type t2) + // Indicates whether the comparison of the 2 specified array types + // requires an object.__equals() lowering. + static bool needsDirectEq(Type t1, Type t2, Scope* sc) { Type t1n = t1.nextOf().toBasetype(); Type t2n = t2.nextOf().toBasetype(); @@ -10988,13 +10990,16 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor Type t = t1n; while (t.toBasetype().nextOf()) t = t.nextOf().toBasetype(); - if (t.ty != Tstruct) - return false; + if (auto ts = t.isTypeStruct()) + { + // semanticTypeInfo() makes sure hasIdentityEquals has been computed + if (global.params.useTypeInfo && Type.dtypeinfo) + semanticTypeInfo(sc, ts); - if (global.params.useTypeInfo && Type.dtypeinfo) - semanticTypeInfo(sc, t); + return ts.sym.hasIdentityEquals; + } - return (cast(TypeStruct)t).sym.hasIdentityEquals; + return false; } if (auto e = exp.op_overload(sc)) @@ -11003,8 +11008,11 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor return; } + const isArrayComparison = (t1.ty == Tarray || t1.ty == Tsarray) && + (t2.ty == Tarray || t2.ty == Tsarray); + const needsArrayLowering = isArrayComparison && needsDirectEq(t1, t2, sc); - if (!(t1.ty == Tarray && t2.ty == Tarray && needsDirectEq(t1, t2))) + if (!needsArrayLowering) { if (auto e = typeCombine(exp, sc)) { @@ -11020,26 +11028,20 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor exp.type = Type.tbool; - // Special handling for array comparisons - if (!(t1.ty == Tarray && t2.ty == Tarray && needsDirectEq(t1, t2))) + if (!isArrayComparison) { - if (!arrayTypeCompatible(exp.loc, exp.e1.type, exp.e2.type)) + if (exp.e1.type != exp.e2.type && exp.e1.type.isfloating() && exp.e2.type.isfloating()) { - if (exp.e1.type != exp.e2.type && exp.e1.type.isfloating() && exp.e2.type.isfloating()) - { - // Cast both to complex - exp.e1 = exp.e1.castTo(sc, Type.tcomplex80); - exp.e2 = exp.e2.castTo(sc, Type.tcomplex80); - } + // Cast both to complex + exp.e1 = exp.e1.castTo(sc, Type.tcomplex80); + exp.e2 = exp.e2.castTo(sc, Type.tcomplex80); } } - if (t1.ty == Tarray && t2.ty == Tarray) + // lower some array comparisons to object.__equals(e1, e2) + if (needsArrayLowering || (t1.ty == Tarray && t2.ty == Tarray)) { - //printf("Lowering to __equals %s %s\n", e1.toChars(), e2.toChars()); - - // For e1 and e2 of struct type, lowers e1 == e2 to object.__equals(e1, e2) - // and e1 != e2 to !(object.__equals(e1, e2)). + //printf("Lowering to __equals %s %s\n", exp.e1.toChars(), exp.e2.toChars()); if (!verifyHookExist(exp.loc, *sc, Id.__equals, "equal checks on arrays")) return setError(); @@ -11058,7 +11060,13 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor { __equals = new NotExp(exp.loc, __equals); } - __equals = __equals.expressionSemantic(sc); + __equals = __equals.trySemantic(sc); // for better error message + if (!__equals) + { + exp.error("incompatible types for array comparison: `%s` and `%s`", + exp.e1.type.toChars(), exp.e2.type.toChars()); + __equals = new ErrorExp(); + } result = __equals; return; diff --git a/dmd/id.d b/dmd/id.d index c04a095b9c7..21b79fc45c3 100644 --- a/dmd/id.d +++ b/dmd/id.d @@ -306,7 +306,6 @@ immutable Msgtable[] msgtable = { "monitorexit", "_d_monitorexit" }, { "criticalenter", "_d_criticalenter" }, { "criticalexit", "_d_criticalexit" }, - { "__ArrayEq" }, { "__ArrayPostblit" }, { "__ArrayDtor" }, { "_d_delThrowable" }, diff --git a/dmd/mtype.h b/dmd/mtype.h index b1d12bda54b..18728855924 100644 --- a/dmd/mtype.h +++ b/dmd/mtype.h @@ -865,7 +865,6 @@ class TypeNull : public Type /**************************************************************/ -bool arrayTypeCompatible(Loc loc, Type *t1, Type *t2); bool arrayTypeCompatibleWithoutCasting(Type *t1, Type *t2); // If the type is a class or struct, returns the symbol for it, else null. diff --git a/dmd/opover.d b/dmd/opover.d index ceaa3d6198d..961b8dd6bae 100644 --- a/dmd/opover.d +++ b/dmd/opover.d @@ -262,7 +262,7 @@ private Expression checkAliasThisForRhs(AggregateDeclaration ad, Scope* sc, BinE * sc = context * pop = if not null, is set to the operator that was actually overloaded, * which may not be `e.op`. Happens when operands are reversed to - * to match an overload + * match an overload * Returns: * `null` if not an operator overload, * otherwise the lowered expression @@ -917,52 +917,14 @@ Expression op_overload(Expression e, Scope* sc, TOK* pop = null) Type t1 = e.e1.type.toBasetype(); Type t2 = e.e2.type.toBasetype(); - /* Check for array equality. + /* Array equality is handled by expressionSemantic() potentially + * lowering to object.__equals(), which takes care of overloaded + * operators for the element types. */ if ((t1.ty == Tarray || t1.ty == Tsarray) && (t2.ty == Tarray || t2.ty == Tsarray)) { - bool needsDirectEq() - { - Type t1n = t1.nextOf().toBasetype(); - Type t2n = t2.nextOf().toBasetype(); - if ((t1n.ty.isSomeChar && t2n.ty.isSomeChar) || - (t1n.ty == Tvoid || t2n.ty == Tvoid)) - { - return false; - } - if (t1n.constOf() != t2n.constOf()) - return true; - - Type t = t1n; - while (t.toBasetype().nextOf()) - t = t.nextOf().toBasetype(); - if (t.ty != Tstruct) - return false; - - if (global.params.useTypeInfo && Type.dtypeinfo) - semanticTypeInfo(sc, t); - - return (cast(TypeStruct)t).sym.hasIdentityEquals; - } - - if (needsDirectEq() && !(t1.ty == Tarray && t2.ty == Tarray)) - { - /* Rewrite as: - * __ArrayEq(e1, e2) - */ - Expression eeq = new IdentifierExp(e.loc, Id.__ArrayEq); - result = new CallExp(e.loc, eeq, e.e1, e.e2); - if (e.op == TOK.notEqual) - result = new NotExp(e.loc, result); - result = result.trySemantic(sc); // for better error message - if (!result) - { - e.error("cannot compare `%s` and `%s`", t1.toChars(), t2.toChars()); - result = new ErrorExp(); - } - return; - } + return; } /* Check for class equality with null literal or typeof(null). @@ -1028,9 +990,6 @@ Expression op_overload(Expression e, Scope* sc, TOK* pop = null) return; } - if (t1.ty == Tarray && t2.ty == Tarray) - return; - /* Check for pointer equality. */ if (t1.ty == Tpointer || t2.ty == Tpointer) diff --git a/runtime/druntime b/runtime/druntime index cc97ccd00d4..bdb41968e2a 160000 --- a/runtime/druntime +++ b/runtime/druntime @@ -1 +1 @@ -Subproject commit cc97ccd00d4082221eee1d5afdbd775201d75877 +Subproject commit bdb41968e2a079b9a88a221542e476d44178f25a From 3e1dcc5a9658088a1f94f34cc31ae6642b1b44a4 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 3 Jun 2020 04:13:51 +0200 Subject: [PATCH 2/3] druntime: Revise core.internal.array.equality --- runtime/druntime | 2 +- tests/baremetal/wasm2.d | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime/druntime b/runtime/druntime index bdb41968e2a..b4b226c64ba 160000 --- a/runtime/druntime +++ b/runtime/druntime @@ -1 +1 @@ -Subproject commit bdb41968e2a079b9a88a221542e476d44178f25a +Subproject commit b4b226c64bac3ff63543db4db5be089621a49db8 diff --git a/tests/baremetal/wasm2.d b/tests/baremetal/wasm2.d index ed94ac8f430..d38e97207b7 100644 --- a/tests/baremetal/wasm2.d +++ b/tests/baremetal/wasm2.d @@ -16,6 +16,8 @@ void _start() {} void __assert(const(char)* msg, const(char)* file, uint line) {} +int memcmp(const void* s1, const void* s2, size_t n) { assert(0); } + export int myExportedFoo() { import std.algorithm, std.range; From ce9b4205a0220a75242b59c939de23cd1fbf3ec8 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Fri, 19 Jun 2020 23:14:53 +0200 Subject: [PATCH 3/3] Don't enforce (normal) emission for pragma(inline, true) function templates Debugging failing fail_compilation/ice19762.d (stack overflow in LDC) was a trip down the rabbit hole. By the looks of it, the root of all evil is again the frontend not properly validating IndexExpressions - this time, for structs with fwd- referencing errors (analogous to the problem for opaque structs with compilable/ice20044.d, see https://issues.dlang.org/show_bug.cgi?id=20959). The stack overflow happens in LLVM when trying to create a GEP for core.internal.array.equality.at(), when checking the struct element type (with cycles, i.e., containing a field of itself) for sizedness. This little function now uses `pragma(inline, true)`, so it was newly codegen'd although needsCodegen() returns false. With the new available_externally emission into each referencing CU, I think this enforced regular emission from the module members tree shouldn't be required anymore, allowing to work around the test regression and possibly more issues with speculative instantiations. After adapting the codegen/inlining_templates.d lit-test accordingly (no IR definitions anymore in .ll file, because available_externally doesn't make it there), I've noticed that a lambda in imported and inlined call_enforce_with_default_template_params() wasn't emitted - it got culled by alreadyOrWillBeDefined(). Function/delegate literals aren't culled anymore. --- gen/declarations.cpp | 33 +++++++++++------------------- gen/function-inlining.cpp | 3 +++ tests/codegen/inlining_templates.d | 10 ++++----- 3 files changed, 19 insertions(+), 27 deletions(-) diff --git a/gen/declarations.cpp b/gen/declarations.cpp index 07bfdc174f0..cc7850b6d23 100644 --- a/gen/declarations.cpp +++ b/gen/declarations.cpp @@ -371,28 +371,19 @@ class CodegenVisitor : public Visitor { return; } - // Force codegen if this is a templated function with pragma(inline, true). - if ((decl->members->length == 1) && - ((*decl->members)[0]->isFuncDeclaration()) && - ((*decl->members)[0]->isFuncDeclaration()->inlining == PINLINEalways)) { - Logger::println("needsCodegen() == false, but function is marked with " - "pragma(inline, true), so it really does need " - "codegen."); - } else { - // FIXME: This is #673 all over again. - if (!decl->needsCodegen()) { - Logger::println("Does not need codegen, skipping."); - return; - } + // FIXME: This is #673 all over again. + if (!decl->needsCodegen()) { + Logger::println("Does not need codegen, skipping."); + return; + } - if (irs->dcomputetarget && (decl->tempdecl == Type::rtinfo || - decl->tempdecl == Type::rtinfoImpl)) { - // Emitting object.RTInfo(Impl) template instantiations in dcompute - // modules would require dcompute support for global variables. - Logger::println("Skipping object.RTInfo(Impl) template instantiations " - "in dcompute modules."); - return; - } + if (irs->dcomputetarget && (decl->tempdecl == Type::rtinfo || + decl->tempdecl == Type::rtinfoImpl)) { + // Emitting object.RTInfo(Impl) template instantiations in dcompute + // modules would require dcompute support for global variables. + Logger::println("Skipping object.RTInfo(Impl) template instantiations " + "in dcompute modules."); + return; } for (auto &m : *decl->members) { diff --git a/gen/function-inlining.cpp b/gen/function-inlining.cpp index fc84fe15246..8d23f32bca5 100644 --- a/gen/function-inlining.cpp +++ b/gen/function-inlining.cpp @@ -73,6 +73,9 @@ bool isInlineCandidate(FuncDeclaration &fdecl) { } // end anonymous namespace bool alreadyOrWillBeDefined(FuncDeclaration &fdecl) { + if (fdecl.isFuncLiteralDeclaration()) // emitted into each referencing CU + return true; + for (FuncDeclaration *f = &fdecl; f;) { if (!f->isInstantiated() && f->inNonRoot()) { return false; diff --git a/tests/codegen/inlining_templates.d b/tests/codegen/inlining_templates.d index 1112e995186..510929d7904 100644 --- a/tests/codegen/inlining_templates.d +++ b/tests/codegen/inlining_templates.d @@ -31,10 +31,8 @@ void main() { } -// CHECK-NOT: declare{{.*}}_D6inputs10inlinables__T12template_fooTiZQrUNaNbNiNfiZi -// CHECK-NOT: declare{{.*}}_D3std9exception__T7enforce +// CHECK-NOT: declare {{.*}}template_foo -// CHECK-DAG: define{{.*}}_D6inputs10inlinables__T12template_fooTiZQrUNaNbNiNfiZi{{.*}}) #[[ATTR:[0-9]+]] -// CHECK-DAG: define{{.*}}_D3std9exception__T7enforce{{.*}}) #[[ATTR]] - -// CHECK-DAG: attributes #[[ATTR]] ={{.*}} alwaysinline +// CHECK-NOT: declare {{.*}}call_enforce_with_default_template_params +// CHECK-NOT: declare {{.*}}__lambda +// CHECK-NOT: declare {{.*}}_D3std9exception__T7enforce