One more pass through __cmp by andralex · Pull Request #1787 · dlang/druntime

andralex · 2017-03-06T20:09:27Z

A few more improvements to __cmp to reduce number of templates generated and tighten the code.

andralex · 2017-03-06T20:09:48Z

cc @WalterBright @somzzz @CyberShadow @MartinNowak

CyberShadow · 2017-03-06T20:33:16Z

Hmm, diff isn't very useful, a bit difficult to review. In such cases splitting changes across multiple commits is helpful.

CyberShadow · 2017-03-06T20:26:56Z

src/core/internal/string.d


 int dstrcmp( scope const char[] s1, scope const char[] s2 ) @trusted
 {
-    import core.stdc.string : memcmp;


This (dstrcmp change) really ought to be in its own commit... currently there is no rationale. I see it's to add CTFE support?

Affirmative.

CyberShadow · 2017-03-06T20:31:16Z

src/object.d

+        // Special-case imaginary types to use comparison for "concrete" types
        static if (is(U1 == ifloat)) alias F = float;
        else static if (is(U1 == idouble)) alias F = double;
        else static if (is(U1 == ireal)) alias F = real;


BTW, should __cmp perhaps have a constraint that restricts the two arguments to have the same unqualified type? The compiler seems to enforce that when you do the comparison, however if we do somehow end up with an instantiation with e.g. float[] and double[], such as an explicit user invocation, then slightly wrong things will happen (such as casting the doubles to floats).

Actually casting entire arrays is more than slightly wrong :)

Currently I planted a static assert at the beginning of __cmp. From the looks of it the qualifiers are passed by the compiler.

From the looks of it the qualifiers are passed by the compiler.

I think that should either be enforced or accounted for, especially if we want to expose this function to users as well (and we do want that, as it would be useful for defining opCmp in user types).

Not sure what the action item is.

@CyberShadow there'd be immutable as well. Doesn't the assert that U1 must be the same as U2 cover it?

Sorry, I don't understand how. Unqual strips immutable so we're still looking at unnecessary template instantiations for all the various constnesses.

@CyberShadow I guess we're talking past each other. What do you want this PR to accomplish?

Let's say we have:

int[] a; const(int)[] b; immutable(int)[] c; writeln(a < b); writeln(b < c); writeln(a < c);

You said that "the qualifiers are passed by the compiler". I understood that to mean that the compiler strips qualifiers before instantiating __cmp, so there will be only one __cmp instantiation? If so, this should be enforced on the __cmp side.

If, however, there are three template instantiations, two of those should short-circuit to calling the third to avoid redundant code gen. I also mentioned this in Lucia's PR and this is what I thought this PR did from your initial description :)

Yah, that's my understanding as well - three copies will be generated. I can work on folding those together in this PR. Stay tuned.

CyberShadow · 2017-03-06T20:36:41Z

src/object.d

+        return (() @trusted => dstrcmp(cast(char[])s1, cast(char[])s2))();
    }
-    // integral, struct, nested arrays
+    // Everything else


The previous comment was more descriptive.

It had become untrue due to the changes in this diff. There are only a few leading special cases for objects and imaginary types, so... "Everything that is not a class/interface or imaginary type..."

I see, but it would still be prudent to be explicit, which documents that we've tried to ensure that the code below should work for the given kinds of types. At this point it is a nitpick though.

CyberShadow · 2017-03-06T20:37:39Z

src/object.d

        foreach (const u; 0 .. len)
        {
-            auto e1 = at(s1, u);
-            auto e2 = at(s2, u);


Good catch, this avoids a copy (and in fact wouldn't work with @disable this(this) structs, would it?)

CyberShadow · 2017-03-06T20:40:59Z

src/object.d

-                    return c < 0 ? -1 : 1;
+                    return c;
            }
            else


Do you think it would make sense to add another static if with if (__traits(compiles, { int i = at(s1, u) - at(s2, u); })?

By the way, one thing we need to keep in mind is efficient comparison of structs that don't have an opCmp, and those that have an auto-generated one (such as when one of the fields has one). That's a thing, right?

The relationship between < and - does not apply to all types.

So does the compiler generate opCmp for structs always/when necessary?

Hmm, doesn't look like it. Curiously, for a struct without opCmp, S(1) < S(2) doesn't compile, but [S(1)] < [S(2)] does. If S has a field with an opCmp, it doesn't get called. So I guess that's not something we need to worry about right now.

Though, there's still the question of structs without opCmp. I think the compiler just does a memcmp right now, but someone would need to check. Ideally we'd want to avoid doing that memcmp twice.

@CyberShadow looks like worth raising a bug report over!

Sorry, what's the bug? Is it for the feature request to auto-generate opCmp?

I'm thinking the fact that S(1) < S(2) does not compile yet [S(1) < S(2)] compiles is definitely a browraiser.

Not [S(1) < S(2)] but [S(1)] < [S(2)]. Is that what you meant?

Oh. Yah, that's problematic too - you can't order lexicographically unless elements are ordered.

https://issues.dlang.org/show_bug.cgi?id=17244

CyberShadow · 2017-03-06T20:43:31Z

A few more improvements to __cmp to reduce number of templates generated and tighten the code.

From that description I thought this would involve making __cmp a stub which normalizes constness and then calls an implementation (or itself). That would make sense, right? So as to avoid multiple instantiations for ubyte[] / ubyte[], ubyte[] / const(ubyte)[], const(ubyte)[] / immutable(ubyte)[]...

andralex · 2017-03-06T20:52:31Z

src/object.d

+    }
+    else static if (is(F == float) || is(F == double) || is(F == real))
+    {
+        return (f1 > f2) - (f2 > f1);


This generates nice code with dmd and gdc, and also much faster than the previous version (and also a bit faster than the obvious alternatives).

WalterBright

This is a good improvement.

andralex · 2017-03-06T23:03:50Z

Had a bit more of a good time taking __cmp out for class/interface objects. Then the generic loop implementation works just fine, no more special casing needed.

CyberShadow · 2017-03-06T23:05:24Z

src/object.d

    {
        return (f1 > f2) - (f2 > f1);
    }
-    else static assert(false, "Internal error");


Why this change? Being explicit is generally better than hoping that the catch-all (else/default) does the right thing.

I'll let folks call __cmp with idouble if they have the need. Just made sure it's not called from the array compare.

CyberShadow · 2017-03-06T23:06:13Z

src/object.d

    enum RTInfo = null;
 }

+// Compare floating point numbers for ordering (one instantiation per width).


I'd suggest enforcing "one instantiation per width" using e.g. static assert(is(Unqual!F == F));

The parameters already have const.

This is a tricky one: the const in the parameters actually does not change F - only the type of f1 / f2.

@CyberShadow got a repro? Just tried this:

void fun(F)(const F x) { pragma(msg, F.stringof); } void main() { float a; fun(a); const float b = 1; fun(b); }

Only one copy is generated.

Ah, OK - it doesn't apply to floats because the compiler strips head const on call. Only types with indirections.

I.e. F will still be non-const but that's not a problem because it will always be non-const :)

CyberShadow · 2017-03-06T23:08:18Z

src/object.d

        {
-            // structs
-            static if (__traits(compiles, at(s1, u).opCmp(at(s2, u))))
+            static if (__traits(compiles, __cmp(at(s1, u), at(s2, u))))


Why the swap? Just curious.

I wanted to make sure __cmp gets first crack so we don't call a.opCmp(b) where a is a class type and its value is null.

somzzz · 2017-03-07T15:31:22Z

I opened the PR for the dmd lowering to __cmp.
dlang/dmd#6597

There are tests failing in DMD because of comparisons of arrays of struct types without opCmp defined. I talked with @andralex and it seems that this is because of an old issue with using memcmp for comparing arrays of structs, which the current implementation doesn't preserve.

A simple example/test:

struct VariantN(size_t maxDataSize)
{
    ~this() {}
}
alias Variant4 = VariantN!(4);
void test()
{
    Variant4[] a4;
    assert(a4 < a4);
}
void main() {}

This could either be fixed here or I could open a new PR after this is merged. What do you think?

…were compared with memcmp

JackStouffer · 2017-03-07T18:20:43Z

@somzzz One thing at a time.

andralex · 2017-03-07T18:22:16Z

@JackStouffer the latest commit should take care of things.

andralex · 2017-03-07T18:42:34Z

src/object.d

+    return lhs.opCmp(rhs);
+}
+
+int __cmp(T)(const T[] lhs, const T[] rhs) @trusted


@CyberShadow ok, so this overload should generate only one copy per scalar type. There are still a couple of opportunities:

merge wchar with short implementation

merge dchar with uint implementation

merge ifloat with float implementation

merge idouble with double implementation

merge ireal with real implementation

I've let these be because they're relatively rare.

ok, so this overload should generate only one copy per scalar type

Are you sure? This doesn't even compile:

int __cmp(T)(const T[] lhs, const T[] rhs) @trusted {} int[] a; immutable(int)[] b; __cmp(a, b);

@CyberShadow take that back, I decided to implement the whole shebang anyway, it doesn't take much code.

worksforme:

void main() { int __cmp(T)(const T[] lhs, const T[] rhs) @trusted { return 0; } int[] a; immutable(int)[] b; __cmp(a, b); }

Nice but constness issue remains, I think that would be good as a test BTW.

…types

CyberShadow · 2017-03-07T18:53:15Z

src/object.d

+    {
+        // Reuse another implementation
+        return __cmp(cast(U[]) lhs, cast(U[]) rhs);
+    }


andralex · 2017-03-08T00:27:17Z

Since this is approved I'll merge.

One more pass through __cmp

6c1c6cf

CyberShadow reviewed Mar 6, 2017

View reviewed changes

andralex commented Mar 6, 2017

View reviewed changes

WalterBright approved these changes Mar 6, 2017

View reviewed changes

Factor in implementation for class and interface types

1fa3903

CyberShadow reviewed Mar 6, 2017

View reviewed changes

andralex added 2 commits March 6, 2017 19:43

Fix __cmp for object types, was not even called

3d9ce7f

Fix copypasta mistake

f3a0e19

Preserve legacy weird behavior by which structs and unions in arrays …

24e775a

…were compared with memcmp

andralex mentioned this pull request Mar 7, 2017

Array Compare Lowering dlang/dmd#6597

Merged

Further reduction of code

7d07225

andralex commented Mar 7, 2017

View reviewed changes

Aw what the heck implemented reuse of implementation for all builtin …

8107d51

…types

CyberShadow reviewed Mar 7, 2017

View reviewed changes

src/object.d

{

// Reuse another implementation

return __cmp(cast(U[]) lhs, cast(U[]) rhs);

}

Copy link

Member

CyberShadow Mar 7, 2017

Choose a reason for hiding this comment

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

👍

Oooops...

fc83c27

andralex merged commit 1b37491 into dlang:master Mar 8, 2017

andralex deleted the __cmp branch March 8, 2017 00:27

JinShil mentioned this pull request Jun 6, 2019

Remove __cmp from object.d for aggregates #2633

Closed

Uh oh!

Conversation

andralex commented Mar 6, 2017

Uh oh!

andralex commented Mar 6, 2017

Uh oh!

CyberShadow commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright left a comment

Choose a reason for hiding this comment

Uh oh!

andralex commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

CyberShadow Mar 6, 2017 •

edited

Loading

somzzz commented Mar 7, 2017 •

edited

Loading

andralex Mar 7, 2017 •

edited

Loading