Skip to content
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
1 change: 0 additions & 1 deletion compiler/src/dmd/backend/drtlsym.d
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ Symbol* getRtlsym(RTLSYM i) @trusted
case RTLSYM.ARRAYCOPY: symbolz(ps,FL.func,FREGSAVED,"_d_arraycopy", 0, t); break;
case RTLSYM.ARRAYASSIGN_R: symbolz(ps,FL.func,FREGSAVED,"_d_arrayassign_r", 0, t); break;
case RTLSYM.ARRAYASSIGN_L: symbolz(ps,FL.func,FREGSAVED,"_d_arrayassign_l", 0, t); break;
case RTLSYM.ARRAYEQ2: symbolz(ps,FL.func,FREGSAVED,"_adEq2", 0, t); break;

/* Associative Arrays https://github.com/dlang/dmd/blob/master/druntime/src/rt/aaA.d */
case RTLSYM.AANEW: symbolz(ps,FL.func,FREGSAVED,"_aaNew", 0, t); break;
Expand Down
28 changes: 9 additions & 19 deletions compiler/src/dmd/e2ir.d
Original file line number Diff line number Diff line change
Expand Up @@ -2105,14 +2105,16 @@ elem* toElem(Expression e, ref IRState irs)
}
else if (t1.isStaticOrDynamicArray() && t2.isStaticOrDynamicArray())
{
Type telement = t1.nextOf().toBasetype();
Type telement2 = t2.nextOf().toBasetype();

if ((telement.isIntegral() || telement.ty == Tvoid) && telement.ty == telement2.ty)
if (auto lowering = ee.lowering)
{
e = toElem(lowering, irs);
elem_setLoc(e, ee.loc);
}
else
{
// Optimize comparisons of arrays of basic types
// For arrays of integers/characters, and void[],
// replace druntime call with:
// For arrays of scalars (except floating types) of same size & signedness, void[],
// and structs with no custom equality operator, replace druntime call with:
// For a==b: a.length==b.length && (a.length == 0 || memcmp(a.ptr, b.ptr, size)==0)
// For a!=b: a.length!=b.length || (a.length != 0 || memcmp(a.ptr, b.ptr, size)!=0)
// size is a.length*sizeof(a[0]) for dynamic arrays, or sizeof(a) for static arrays.
Expand All @@ -2122,7 +2124,7 @@ elem* toElem(Expression e, ref IRState irs)
elem* eptr1, eptr2; // Pointer to data, to pass to memcmp
elem* elen1, elen2; // Length, for comparison
elem* esiz1, esiz2; // Data size, to pass to memcmp
const sz = telement.size(); // Size of one element
const sz = t1.nextOf().toBasetype().size(); // Size of one element

bool is64 = target.isX86_64 || target.isAArch64;
if (t1.ty == Tarray)
Expand Down Expand Up @@ -2175,19 +2177,7 @@ elem* toElem(Expression e, ref IRState irs)
e = el_combine(earr2, e);
e = el_combine(earr1, e);
elem_setLoc(e, ee.loc);
return e;
}

elem* ea1 = eval_Darray(ee.e1);
elem* ea2 = eval_Darray(ee.e2);

elem* ep = el_params(getTypeInfo(ee, telement.arrayOf(), irs),
ea2, ea1, null);
const rtlfunc = RTLSYM.ARRAYEQ2;
e = el_bin(OPcall, TYint, el_var(getRtlsym(rtlfunc)), ep);
if (ee.op == EXP.notEqual)
e = el_bin(OPxor, TYint, e, el_long(TYint, 1));
elem_setLoc(e,ee.loc);
}
else if (t1.ty == Taarray && t2.ty == Taarray)
{
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dmd/expression.d
Original file line number Diff line number Diff line change
Expand Up @@ -4802,6 +4802,7 @@ extern (C++) final class RemoveExp : BinExp
*/
extern (C++) final class EqualExp : BinExp
{
Expression lowering;
extern (D) this(EXP op, Loc loc, Expression e1, Expression e2) @safe
{
super(loc, op, e1, e2);
Expand Down
2 changes: 2 additions & 0 deletions compiler/src/dmd/expression.h
Original file line number Diff line number Diff line change
Expand Up @@ -1280,6 +1280,8 @@ class RemoveExp final : public BinExp
class EqualExp final : public BinExp
{
public:
Expression* lowering;

void accept(Visitor *v) override { v->visit(this); }
};

Expand Down
192 changes: 170 additions & 22 deletions compiler/src/dmd/expressionsem.d
Original file line number Diff line number Diff line change
Expand Up @@ -3860,6 +3860,78 @@
cex.lowering = lowering.expressionSemantic(sc);
}

/**
* Visitor to check if a type is suitable for comparison using `memcmp`.
* Currently used when lowering `EqualExp`.
*/
private extern(C++) final class IsMemcmpableVisitor : Visitor
{
alias visit = Visitor.visit;
public:
bool result = false;

override void visit(Type t)
{
result = t.ty == Tvoid || (t.isScalar() && !t.isFloating());
}

override void visit(TypeStruct ts)
{
result = false;

if (ts.sym.hasIdentityEquals)
return; // has custom opEquals

if (!ts.sym.members)
{
result = true;
return;
}

/* We recursively check all variable declaration within the struct.
* The recursiveness is needed to handle cases like this:
* struct Test {
* nothrow:
* int[] contents;
* }
* Here a `StorageClassDeclaration` symbol will be created, which wraps the variable declaration.
*/
static bool visitAllMembers(Dsymbols* members, TypeStruct root, IsMemcmpableVisitor v)
{
if (members is null)
return true;

Check warning on line 3902 in compiler/src/dmd/expressionsem.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/expressionsem.d#L3902

Added line #L3902 was not covered by tests

foreach (m; *members)
{
if (auto vd = m.isVarDeclaration())
{
if (vd.type is null)
continue;

Check warning on line 3909 in compiler/src/dmd/expressionsem.d

View check run for this annotation

Codecov / codecov/patch

compiler/src/dmd/expressionsem.d#L3909

Added line #L3909 was not covered by tests

auto tbvd = vd.type.toBasetype();
if (tbvd !is root)
tbvd.accept(v);

if (!v.result)
return false;
}
else if (auto ad = m.isAttribDeclaration())
{
if(!visitAllMembers(ad.decl, root, v))
return false;
}
}
return true;
}
result = visitAllMembers(ts.sym.members, ts, this);
}

override void visit(TypeSArray tsa)
{
tsa.nextOf().toBasetype().accept(this);
}
}

private extern (C++) final class ExpressionSemanticVisitor : Visitor
{
alias visit = Visitor.visit;
Expand Down Expand Up @@ -13412,19 +13484,53 @@
Type t1 = exp.e1.type.toBasetype();
Type t2 = exp.e2.type.toBasetype();

// Indicates whether the comparison of the 2 specified array types
// requires an object.__equals() lowering.
static bool needsDirectEq(Type t1, Type t2, Scope* sc)
static bool unifyArrayTypes(Type t1, Type t2, Scope* sc)
{
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;
return true;
}
if (t1n.constOf() != t2n.constOf())
{
return false;
}

Type t = t1n;
while (t.toBasetype().nextOf())
t = t.nextOf().toBasetype();
if (auto ts = t.isTypeStruct())
{
// semanticTypeInfo() makes sure hasIdentityEquals has been computed
if (global.params.useTypeInfo && Type.dtypeinfo)
semanticTypeInfo(sc, ts);

return !ts.sym.hasIdentityEquals; // has custom opEquals
}

return true;
}

static bool shouldUseMemcmp(Type t1, Type t2, Scope *sc)
{
Type t1n = t1.nextOf().toBasetype();
Type t2n = t2.nextOf().toBasetype();
const t1nsz = t1n.size();
const t2nsz = t2n.size();

if ((t1n.ty == Tvoid || (t1n.isScalar() && !t1n.isFloating())) &&
(t2n.ty == Tvoid || (t2n.isScalar() && !t1n.isFloating())) &&
t1nsz == t2nsz && t1n.isUnsigned() == t2n.isUnsigned())
{
return true;
}
if (t1n.constOf() != t2n.constOf())
{
return false;
}

Type t = t1n;
while (t.toBasetype().nextOf())
Expand All @@ -13435,7 +13541,9 @@
if (global.params.useTypeInfo && Type.dtypeinfo)
semanticTypeInfo(sc, ts);

return ts.sym.hasIdentityEquals; // has custom opEquals
auto v = new IsMemcmpableVisitor();
ts.accept(v);
return v.result;
}

return false;
Expand All @@ -13448,9 +13556,8 @@
}

const isArrayComparison = t1.isStaticOrDynamicArray() && t2.isStaticOrDynamicArray();
const needsArrayLowering = isArrayComparison && needsDirectEq(t1, t2, sc);

if (!needsArrayLowering)
if (!isArrayComparison || unifyArrayTypes(t1, t2, sc))
{
// https://issues.dlang.org/show_bug.cgi?id=23783
if (exp.e1.checkSharedAccess(sc) || exp.e2.checkSharedAccess(sc))
Expand Down Expand Up @@ -13480,7 +13587,7 @@
}

// lower some array comparisons to object.__equals(e1, e2)
if (needsArrayLowering || (t1.ty == Tarray && t2.ty == Tarray))
if (isArrayComparison && !shouldUseMemcmp(t1, t2, sc))
{
//printf("Lowering to __equals %s %s\n", exp.e1.toChars(), exp.e2.toChars());

Expand All @@ -13495,13 +13602,13 @@
return;
}

if (!verifyHookExist(exp.loc, *sc, Id.__equals, "equal checks on arrays"))
Identifier hook = Id.__equals;
if (!verifyHookExist(exp.loc, *sc, hook, "equal checks on arrays"))
return setError();

Expression __equals = new IdentifierExp(exp.loc, Id.empty);
Identifier id = Identifier.idPool("__equals");
__equals = new DotIdExp(exp.loc, __equals, Id.object);
__equals = new DotIdExp(exp.loc, __equals, id);
Expression lowering = new IdentifierExp(exp.loc, Id.empty);
lowering = new DotIdExp(exp.loc, lowering, Id.object);
lowering = new DotIdExp(exp.loc, lowering, hook);

/* https://issues.dlang.org/show_bug.cgi?id=23674
*
Expand All @@ -13512,30 +13619,71 @@
exp.e1 = exp.e1.optimize(WANTvalue);
exp.e2 = exp.e2.optimize(WANTvalue);

auto e1c = exp.e1.copy();
auto e2c = exp.e2.copy();

/* Remove qualifiers from the types of the arguments to reduce the number
* of generated `__equals` instances.
*/
static void unqualifyExp(Expression e)
{
e.type = e.type.unqualify(MODFlags.wild | MODFlags.immutable_ | MODFlags.shared_);
auto eNext = e.type.nextOf();
if (eNext && !eNext.toBasetype().isTypeStruct())
e.type = e.type.unqualify(MODFlags.const_);

if (!e.isArrayLiteralExp())
return;

if (auto elems = e.isArrayLiteralExp().elements)
foreach(elem; *elems)
if (elem)
unqualifyExp(elem);
}
unqualifyExp(e1c);
unqualifyExp(e2c);

auto arguments = new Expressions(2);
(*arguments)[0] = exp.e1;
(*arguments)[1] = exp.e2;
(*arguments)[0] = e1c;
(*arguments)[1] = e2c;

__equals = new CallExp(exp.loc, __equals, arguments);
lowering = new CallExp(exp.loc, lowering, arguments);
if (exp.op == EXP.notEqual)
{
__equals = new NotExp(exp.loc, __equals);
lowering = new NotExp(exp.loc, lowering);
}
__equals = __equals.trySemantic(sc); // for better error message
if (!__equals)
lowering = lowering.trySemantic(sc); // for better error message
if (!lowering)
{
if (sc.func)
error(exp.loc, "can't infer return type in function `%s`", sc.func.toChars());
else
error(exp.loc, "incompatible types for array comparison: `%s` and `%s`",
exp.e1.type.toChars(), exp.e2.type.toChars());
__equals = ErrorExp.get();
lowering = ErrorExp.get();
}

result = __equals;
exp.lowering = lowering;
result = exp;
return;
}

// When array comparison is not lowered to `__equals`, `memcmp` is used, but
// GC checks occur before the expression is lowered to `memcmp` in e2ir.d.
// Thus, we will consider the literal arrays as on-stack arrays to avoid issues
// during GC checks.
if (isArrayComparison)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thewilsonator Because of the fact that now the result expression is the original one with a .lowering field that is set to null when memcmp is used (the lowering to memcmp happens in e2ir.d), the GC checks will be done on the original expression. This hack allows to preserve the current upstream behavior by avoiding flagging the array literals as @gc. Is there a better way of achieving the same goal, but without this type of hacks?

{
if (auto ale1 = exp.e1.isArrayLiteralExp())
{
ale1.onstack = true;
}

if (auto ale2 = exp.e2.isArrayLiteralExp())
{
ale2.onstack = true;
}
}

if (exp.e1.type.toBasetype().ty == Taarray)
{
semanticTypeInfo(sc, exp.e1.type.toBasetype());
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dmd/frontend.h
Original file line number Diff line number Diff line change
Expand Up @@ -2987,6 +2987,7 @@ class DsymbolExp final : public Expression
class EqualExp final : public BinExp
{
public:
Expression* lowering;
void accept(Visitor* v) override;
};

Expand Down
23 changes: 22 additions & 1 deletion compiler/src/dmd/inline.d
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,16 @@ public:

override void visit(EqualExp e)
{
visit(cast(BinExp)e);
auto ee = cast(EqualExp)e.copy();
if (auto lowering = ee.lowering)
{
ee.lowering = doInlineAs!Expression(lowering, ids);
}

ee.e1 = doInlineAs!Expression(e.e1, ids);
ee.e2 = doInlineAs!Expression(e.e2, ids);

result = ee;

Type t1 = e.e1.type.toBasetype();
if (t1.isStaticOrDynamicArray())
Expand Down Expand Up @@ -1336,6 +1345,18 @@ public:
inlineScan(e.e2);
}

override void visit(EqualExp e)
{
if (auto lowering = e.lowering)
{
inlineScan(lowering);
}
else
{
visit(cast(BinExp)e);
}
}

override void visit(AssignExp e)
{
// Look for NRVO, as inlining NRVO function returns require special handling
Expand Down
Loading
Loading