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.

Sign up for GitHub

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

Lower CmpExp between classes to __cmp call #9629

Merged
merged 1 commit into from
Jan 3, 2022
+64 −5
Merged

Lower CmpExp between classes to __cmp call #9629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 35 additions & 1 deletion src/dmd/expressionsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11333,7 +11333,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return setError();
}

EXP cmpop;

EXP cmpop = exp.op;
if (auto e = exp.op_overload(sc, &cmpop))
{
if (!e.type.isscalar() && e.type.equals(exp.e1.type))
Expand All @@ -11343,13 +11344,46 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
}
if (e.op == EXP.call)
{

if (t1.ty == Tclass && t2.ty == Tclass)
Copy link
Member

Choose a reason for hiding this comment

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

I think you also have to verify that the call is actually to Object.opCmp, as it can be redirected to other functions aswell, e.g.

class A
{
    int x;
    this(int a) { x = a; }
    
    alias opCmp = Object.opCmp;
    alias opCmp = my_cmp;
    
    final int my_cmp(A a)
    {
        return x - a.x;
    }
}

int main()
{
    auto a = new A(1);    
    return a < a;
}

The comparison currently results in a non-virtual call to my_cmp. Does the lowered template do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lowered template doest the same. I don't think the lowering should change the current behaviour.
Why do you want the behaviour to be different?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you want the behaviour to be different?

I don't want observable changes, and AFAICT the actual call stays the same - that's good. But the template adds some additional checks that are not there so far. This might duplicate null checks in existing custom implementations of opCmp, or it might work differently. The spec doesn't seem to cover the comparison to null. I'm not against adding the checks, just saying they should be added to the spec (similar to opEquals) if the change is deliberate.

BTW: dmd is unable to inline the __cmp template, it can if it is written as return lhs is rhs ? 0 : lhs is null ? -1 : rhs is null ? 1 : lhs.opCmp(rhs);. dmd doesn't eliminate unnecessary checks, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want the behaviour to be different?

I don't want observable changes, and AFAICT the actual call stays the same - that's good. But the template adds some additional checks that are not there so far. This might duplicate null checks in existing custom implementations of opCmp, or it might work differently. The spec doesn't seem to cover the comparison to null. I'm not against adding the checks, just saying they should be added to the spec (similar to opEquals) if the change is deliberate.

The change is deliberate. I’ll update the spec to mention the changes.

BTW: dmd is unable to inline the __cmp template, it can if it is written as return lhs is rhs ? 0 : lhs is null ? -1 : rhs is null ? 1 : lhs.opCmp(rhs);. dmd doesn't eliminate unnecessary checks, though.

Hmm, rewritting it this way would make the code much harder to understand. Imho, we shouldn’t make the code harder to understand just so we improve dmd’s inlining. If this also affects ldc and gdc (haven’t checked yet) then we should modify the call.

I think the issue at hand is related to how dmd is able to inline such constructs. I think that should be a separate improvement done in dmd, as it will benefit other existing user code bases.

{
// Lower to object.__cmp(e1, e2)
Expression cl = new IdentifierExp(exp.loc, Id.empty);
cl = new DotIdExp(exp.loc, cl, Id.object);
cl = new DotIdExp(exp.loc, cl, Id.__cmp);
cl = cl.expressionSemantic(sc);

auto arguments = new Expressions();
// Check if op_overload found a better match by calling e2.opCmp(e1)
// If the operands were swapped, then the result must be reversed
// e1.opCmp(e2) == -e2.opCmp(e1)
// cmpop takes care of this
if (exp.op == cmpop)
{
arguments.push(exp.e1);
arguments.push(exp.e2);
}
else
{
// Use better match found by op_overload
arguments.push(exp.e2);
arguments.push(exp.e1);
}

cl = new CallExp(exp.loc, cl, arguments);
cl = new CmpExp(cmpop, exp.loc, cl, new IntegerExp(0));
result = cl.expressionSemantic(sc);
return;
}

e = new CmpExp(cmpop, exp.loc, e, IntegerExp.literal!0);
e = e.expressionSemantic(sc);
}
result = e;
return;
}


if (Expression ex = typeCombine(exp, sc))
{
result = ex;
Expand Down
8 changes: 4 additions & 4 deletions src/dmd/opover.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,11 +853,11 @@ Expression op_overload(Expression e, Scope* sc, EXP* pop = null)
{
// Rewrite (e1 op e2) as e2.opfunc(e1)
result = build_overload(e.loc, sc, e.e2, e.e1, m.lastf ? m.lastf : s);
// When reversing operands of comparison operators,
// need to reverse the sense of the op
if (pop)
*pop = reverseRelation(e.op);
}
// When reversing operands of comparison operators,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I belive it is: you only need to reverse the op only when we decide to write the call exp as e2.opFunc(e1).
As it was, the op was always reversed. Imho this was a bug that just wasn’t manifesting

// need to reverse the sense of the op
if (pop)
*pop = reverseRelation(e.op);
return;
}
}
Expand Down
25 changes: 25 additions & 0 deletions test/runnable/class_opCmp.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
class A
{
int x;
this(int a) { x = a; }

alias opCmp = Object.opCmp;
alias opCmp = my_cmp;

final int my_cmp(A a)
{
return x - a.x;
}
}

void main()
{
auto a1 = new A(1);
auto a2 = new A(2);
A a_null = null;
assert(a1 > a_null);
assert(a_null < a1);
assert(!(a1 < a1));
assert(a1 < a2);
assert(a2 > a1);
}