Lower CmpExp between classes to __cmp call #9629

edi33416 · 2019-04-16T21:06:54Z

This requires druntime's PR

dlang-bot · 2019-04-16T21:06:56Z

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#9629"

wilzbach

Can we test this with a fake object.d directly here?

wilzbach · 2019-04-16T23:08:25Z

src/dmd/expressionsem.d

+            // 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);


As discussed on other PRs: don't use __ as it can lead to weird side effects - use _d_

__cmp is already defined in druntime and it’s already used in the lowering of array comparisons. This PR just uses an overload of it.

@wilzbach In this case the Id.__cmp is unavoidable. It's the same for the __ArrayCast identifier that I refactored in #9572. We'd have to refactor id.d and object.d to correct the issue. See how they are mixed in here: https://github.com/JinShil/dmd/blob/master/src/dmd/id.d#L26-L42

If this were a new symbol, it would be prudent to have it changed, but since this already exists, I think it would be too disruptive to include it in this PR. It could be remedied in a separate DMD and druntime PR, however.

edi33416 · 2019-04-16T23:32:58Z

Can we test this with a fake object.d directly here?

How can I do that?

JinShil · 2019-04-17T00:08:15Z

Can we test this with a fake object.d directly here?

What I've been advocating (actually an idea from @ibuclaw) is to focus on the druntime PR first.

The druntime PR should be dead code except for some unittests that specifically test the new implementation.
After the dead code is committed to druntime, the DMD PR can officially test it.
After the DMD PR is merged, a followup PR can be submitted to druntime to remove the code that the original druntime PR replaced.

So, I suggest focusing on the druntime PR first:
To test the new druntime code, add some unittests to the druntime PR that call the __cmp function directly. This can help demonstrate the druntime code is doing what it should before focusing on the DMD PR, and reduce the likelihood of a messy back-and-forth updating druntime and DMD until all problems are sorted out. See the unittests at https://github.com/dlang/druntime/blob/50d8d0910f2b886a09fa0cff4102db37e33655bc/src/object.d#L4859-L4879 for a demonstration.

Here's the workflow we followed for changing _d_arraycast to __ArrayCast:

druntime PR to add object.__ArrayCast as dead code: Convert _d_arraycast to template druntime#2264
DMD PR to replace call to _d_arraycast with instantiation of object__ArrayCast (object.__ArrayCast is no longer dead code): Replace call to runtime hook _d_arraycast with call to template object.__ArrayCast - Take 2 #9516
druntime PR to remove old _d_arraycast: Remove _d_arraycast druntime#2535

There were a couple of patches along the way, but I think that's a good pattern to follow for this work.

Of course it would be wise for the author to test locally before submitting any PRs to reduce additional PRs being needed to patch things.

jacob-carlborg · 2019-04-17T05:09:49Z

LGTM.

jacob-carlborg · 2019-04-17T09:48:32Z

src/dmd/expressionsem.d

+        if (t1.ty == Tclass && t2.ty == Tclass)
+        {
+            // Lower to object.__cmp(e1, e2)
+            Expression cl = new IdentifierExp(exp.loc, Id.empty);


Will this lower to object.__cmp(e1, e2) or .object.__cmp(e1, e2)? Note the leading dot.

Is there a .object.__cmp(e1, e2) ? I assumed that object.__cmp(e1, e2) gives the full path, so the dot(.) for "take the global __cmp(e1, e2)" wouldn't change who gets called. Am I wrong?

object.__cmp(e1, e2) could find a local class or instance object and search it for a member __cmp, but Id.empty takes care of starting the search at module level. So .object.__cmp(e1, e2) would be a more accurate comment, but that is not done anywhere else.

but that is not done anywhere else

It doesn't hurt to improve. I had to ask so it was not clear enough.

rainers · 2019-04-18T06:10:00Z

src/dmd/expressionsem.d

@@ -10225,6 +10225,24 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
            return setError();
        }

+        if (t1.ty == Tclass && t2.ty == Tclass)


I suspect this is handled too early here. What about operator overloads, e.g. opBinary and opBinaryRight?

IMO lowerings should happen after all possible semantic errors have been reported to the user, so the user doesn't see cryptic messages referring to the lowered expressions in druntime (unless the implementation there is broken).

I suspect this is handled too early here. IMO lowerings should happen after all possible semantic errors have been reported to the user, so the user doesn't see cryptic messages referring to the lowered expressions in druntime (unless the implementation there is broken).

Where, specifically, do you recommend this be done? How does one know that semantic is finished and it's the proper time for lowering?

What about operator overloads, e.g. opBinary and opBinaryRight?

Can you provide a test case?

Sorry for the delay.

Where, specifically, do you recommend this be done?

I would have expected it to be close to where the array lowering is happening, i.e. after operator overload.

How does one know that semantic is finished and it's the proper time for lowering?

Yeah, hard to tell due to possibly deferred analysis. I imagine/dream that no lowerings should happen in the first 3 semantic passes (so user messages never get polluted with compiler generated identifiers and the AST can be mapped back to the source correctly). An additional pass would lower the code to library constructs and continue semantic analysis on the result, but should never generate messages if the library code isn't screwed up.

What about operator overloads, e.g. opBinary and opBinaryRight?
Can you provide a test case?

I was wrong about opBinary that cannot replace opCmp, but here's an example that gets lowered too early:

import core.stdc.stdio; class A { int a = 3; override int opCmp(Object o) { auto b = cast(B)o; if (!b) return super.opCmp(o); printf("comparing a=%d with b=%d\n", this.a, b.b); return this.a - b.b; } } class B { int b = 4; } void main() { A a = new A; B b = new B; assert(b > a); }

I don't think it is possible for the library code to detect "rewrite 2" in https://dlang.org/spec/operatoroverloading.html#compare

Yeah, hard to tell due to possibly deferred analysis.

Doesn't the semantic analysis end here:

dmd/src/dmd/mars.d

Line 619 in 40d85cf

Module.runDeferredSemantic3();

Or possibly here:

dmd/src/dmd/mars.d

Line 653 in 40d85cf

}

n8sh · 2019-05-14T14:26:38Z

If I understand correctly, the purpose of this PR is so that comparison will work with null, right?

andralex

@rainers or @WalterBright when you're good I'm good. Please request changes/approve appropriately.

rainers

comment https://github.com/dlang/dmd/pull/9629/files#r277529334 not addressed yet.

RazvanN7

cc @rainers I think this is good to go

RazvanN7 · 2021-12-17T17:07:28Z

src/dmd/opover.d

                        }
-                        // When reversing operands of comparison operators,


Is this change necessary? Why?

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

src/dmd/expressionsem.d

rainers · 2021-12-18T13:45:22Z

cc @rainers I think this is good to go

The global symbol lookup seems fine now, but none of the changes seem to be covered by the test suite. As the druntime PR is merged, adding tests should be possible now.

RazvanN7 · 2021-12-18T13:56:36Z

@rainers Hmm, I was going to write that since this implementation is now used for class comparison, the fact that the tests are passing shows that it should be correct. My expectation wass that class comparisons are thoroughly tested in the test suite, but upon a transitory look, I was surprised to see how little such tests are employed. @edi33416 could you please add tests as to cover the added code?

rainers · 2021-12-18T13:58:30Z

src/dmd/expressionsem.d

@@ -11343,13 +11344,48 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
            }
            if (e.op == EXP.call)
            {
+
+                if (t1.ty == Tclass && t2.ty == Tclass)


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?

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?

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.

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.

RazvanN7 · 2021-12-22T08:52:07Z

@rainers Is this good to go?

rainers

Still not very happy with replacing any call with the template (see ExpressionSemanticVisitor.visit(CallExp e) for what might happen to the expression), but the existing code assumes the same. So probably good enough.

BTW: it's a bit strange that comparison to null is supported by the template, but forbidden when explicit null is used (see error message a couple of lines above). Will this restriction be removed, too?

12345swordy · 2021-12-24T01:06:40Z

Still not very happy with replacing any call with the template (see ExpressionSemanticVisitor.visit(CallExp e) for what might happen to the expression), but the existing code assumes the same. So probably good enough.

BTW: it's a bit strange that comparison to null is supported by the template, but forbidden when explicit null is used (see error message a couple of lines above). Will this restriction be removed, too?

I hope so, as the current restriction doesn't make any sense.

wilzbach reviewed Apr 16, 2019

View reviewed changes

jacob-carlborg reviewed Apr 17, 2019

View reviewed changes

rainers reviewed Apr 18, 2019

View reviewed changes

edi33416 mentioned this pull request Jun 8, 2019

Remove __cmp from object.d for aggregates dlang/druntime#2633

Closed

andralex reviewed Jun 9, 2019

View reviewed changes

rainers requested changes Jun 12, 2019

View reviewed changes

andralex mentioned this pull request Jul 25, 2020

No need for dstrcmp in object.d dlang/druntime#3165

Merged

dlang-bot added Needs Rebase Needs Work stalled labels May 20, 2021

edi33416 force-pushed the lower_class_CmpExp branch from d1786b6 to 4fbcb6c Compare December 14, 2021 16:42

dlang-bot added Needs Work stalled and removed Needs Work Needs Rebase stalled labels Dec 14, 2021

edi33416 force-pushed the lower_class_CmpExp branch from 4fbcb6c to 281b49e Compare December 17, 2021 13:07

dlang-bot removed Needs Work stalled labels Dec 17, 2021

edi33416 force-pushed the lower_class_CmpExp branch 2 times, most recently from 5fb6afc to ff37800 Compare December 17, 2021 15:23

dlang-bot added the stalled label Dec 17, 2021

edi33416 force-pushed the lower_class_CmpExp branch from ff37800 to afcbe15 Compare December 17, 2021 16:24

dlang-bot removed the stalled label Dec 17, 2021

RazvanN7 approved these changes Dec 17, 2021

View reviewed changes

RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Dec 17, 2021

RazvanN7 reviewed Dec 17, 2021

View reviewed changes

RazvanN7 reviewed Dec 18, 2021

View reviewed changes

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

rainers reviewed Dec 18, 2021

View reviewed changes

edi33416 force-pushed the lower_class_CmpExp branch from afcbe15 to 6ed5af8 Compare December 19, 2021 14:21

Lower CmpExp between classes to __cmp call

c68758e

edi33416 force-pushed the lower_class_CmpExp branch from 6ed5af8 to c68758e Compare December 19, 2021 18:39

rainers approved these changes Dec 23, 2021

View reviewed changes

RazvanN7 merged commit 9bd87f7 into dlang:master Jan 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower CmpExp between classes to __cmp call #9629

Lower CmpExp between classes to __cmp call #9629

edi33416 commented Apr 16, 2019

dlang-bot commented Apr 16, 2019 •

edited

Loading

wilzbach left a comment

wilzbach Apr 16, 2019

edi33416 Apr 16, 2019

JinShil Apr 16, 2019

edi33416 commented Apr 16, 2019

JinShil commented Apr 17, 2019 •

edited

Loading

jacob-carlborg commented Apr 17, 2019

jacob-carlborg Apr 17, 2019

edi33416 Apr 17, 2019

rainers Apr 18, 2019

jacob-carlborg Apr 18, 2019

rainers Apr 18, 2019

JinShil Apr 19, 2019

rainers Apr 23, 2019

jacob-carlborg Apr 23, 2019

n8sh commented May 14, 2019

andralex left a comment

rainers left a comment

RazvanN7 left a comment

RazvanN7 Dec 17, 2021

edi33416 Dec 18, 2021

rainers commented Dec 18, 2021

RazvanN7 commented Dec 18, 2021

rainers Dec 18, 2021

edi33416 Dec 19, 2021

rainers Dec 20, 2021

edi33416 Dec 21, 2021

RazvanN7 commented Dec 22, 2021

rainers left a comment

12345swordy commented Dec 24, 2021

Lower CmpExp between classes to __cmp call #9629

Lower CmpExp between classes to __cmp call #9629

Conversation

edi33416 commented Apr 16, 2019

dlang-bot commented Apr 16, 2019 • edited Loading

Bugzilla references

Testing this PR locally

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edi33416 commented Apr 16, 2019

JinShil commented Apr 17, 2019 • edited Loading

jacob-carlborg commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n8sh commented May 14, 2019

andralex left a comment

Choose a reason for hiding this comment

rainers left a comment

Choose a reason for hiding this comment

RazvanN7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Dec 18, 2021

RazvanN7 commented Dec 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented Dec 22, 2021

rainers left a comment

Choose a reason for hiding this comment

12345swordy commented Dec 24, 2021

dlang-bot commented Apr 16, 2019 •

edited

Loading

JinShil commented Apr 17, 2019 •

edited

Loading