Fix Issue 22082 - Static float array comparison now lowered to __equals #13320

ryuukk · 2021-11-19T03:05:38Z

No description provided.

dlang-bot · 2021-11-19T03:05:43Z

Thanks for your pull request and interest in making D better, @ryuukk! 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

Auto-close	Bugzilla	Severity	Description
✓	22082	normal	static float array comparison not available in betterC

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#13320"

Solve this: ```D struct VertexB { float[2] pos; } struct Hold { VertexB v; } extern(C) int main() { Hold[1] h_a; Hold[1] h_b; assert(h_a != h_b); }

JohanEngelen · 2021-11-19T11:44:56Z

test/compilable/b22082.d

+
+struct VertexA
+{
+    float pos;


All floating point types should be tested, not only float.

i tried to update the test but i'm getting the following:

test.obj : error LNK2019: unresolved external symbol _memset80 referenced in function _D4test__TQiTeZQnFNaNbNiNfZv test.exe : fatal error LNK1120: 1 unresolved externals Error: linker exited with status 1120

updated test:

// PERMUTE_ARGS: // REQUIRED_ARGS: -betterC struct VertexA(T) { T pos; } struct VertexB(T) { T[2] pos; } void test(T)() { T f_a; T f_b; assert(f_a != f_b); f_a = 0; assert(f_a != f_b); f_b = 0; assert(f_a == f_b); VertexA!T a_a; VertexA!T a_b; assert(a_a != a_b); a_a.pos = 0; assert(a_a != a_b); a_b.pos = 0; assert(a_a == a_b); VertexB!T b_a; VertexB!T b_b; assert(b_a != b_b); b_a.pos = 0; assert(b_a != b_b); b_b.pos = 0; assert(b_a == b_b); } extern(C) int main() { test!(float)(); test!(double)(); test!(real)(); return 0; }

JohanEngelen · 2021-11-19T11:48:31Z

src/dmd/expressionsem.d

@@ -11486,6 +11486,10 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
            {
                return false;
            }
+
+            if (t1n.isfloating() && t2n.isfloating())


I find this a bit arbitrary... shouldn't there be a test whether it's a scalar or not? Why is there no test needed here for int for example?

because for integral types it already get optimized to use mem_cmp

for floating points it can't, and fallsback to using TypeInfo, wich is not available in betterC / wasm

dmd/src/dmd/e2ir.d

Line 1865 in 831d407

if ((telement.isintegral() || telement.ty == Tvoid) && telement.ty == telement2.ty)

ryuukk

I added the definition of the missing symbol, i suspect it's simply because betterC doesn't link with the runtime, so we need to add it ourselves, ideally this shouldn't be needed, but i don't know where to put it in dmd?

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

ryuukk · 2021-11-19T13:52:24Z

test/compilable/b22082.d

+
+struct VertexA
+{
+    float pos;


i tried to update the test but i'm getting the following:

test.obj : error LNK2019: unresolved external symbol _memset80 referenced in function _D4test__TQiTeZQnFNaNbNiNfZv test.exe : fatal error LNK1120: 1 unresolved externals Error: linker exited with status 1120

updated test:

// PERMUTE_ARGS: // REQUIRED_ARGS: -betterC struct VertexA(T) { T pos; } struct VertexB(T) { T[2] pos; } void test(T)() { T f_a; T f_b; assert(f_a != f_b); f_a = 0; assert(f_a != f_b); f_b = 0; assert(f_a == f_b); VertexA!T a_a; VertexA!T a_b; assert(a_a != a_b); a_a.pos = 0; assert(a_a != a_b); a_b.pos = 0; assert(a_a == a_b); VertexB!T b_a; VertexB!T b_b; assert(b_a != b_b); b_a.pos = 0; assert(b_a != b_b); b_b.pos = 0; assert(b_a == b_b); } extern(C) int main() { test!(float)(); test!(double)(); test!(real)(); return 0; }

kinke · 2021-11-19T16:23:59Z

src/dmd/expressionsem.d

@@ -11542,7 +11546,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
        }

        // lower some array comparisons to object.__equals(e1, e2)
-        if (needsArrayLowering || (t1.ty == Tarray && t2.ty == Tarray))
+        if (needsArrayLowering || (t1.ty == Tarray && t2.ty == Tarray) || (t1.ty == Tsarray && t2.ty == Tsarray))


This is most likely too much, lowering all equality comparisons of static array pairs.

That's because this test otherwise doesn't work:

struct VertexB { float[2] pos; } struct Hold { VertexB v; } extern(C) int main() { Hold[1] h_a; Hold[1] h_b; assert(h_a != h_b); }

kinke · 2021-11-19T16:34:06Z

IIRC, I've at point tried to lower all array equality comparisons to the template, but that didn't work due to string special cases.

In general, https://github.com/dlang/druntime/blob/master/src/core/internal/array/equality.d should be able to handle all array pairs, and use memcmp if possible. For static array pairs, it would probably add a superfluous length check, but that's probably negligible, incl. the static-array-to-slice promotion.

Currently, the lowering is avoided in some cases, especially for matching integral elements with memcmp-ability, and the equality expression forwarded to the glue layer, where each compiler has its own checks for memcmp-ability and tries to do that, otherwise falling back to the slow, TypeInfo-based _adEq2 druntime helper. That's quite redundant (each compiler, and memcmp-logic also in druntime) but faster to compile without template bloat, and guaranteed inlined, contrary to the template, which might require LTO for inlining length check and memcmp call. So that's the trade-off.

ryuukk · 2021-11-19T22:14:15Z

Well all i wanted was to make static float array to work.. that's blocking my project right now.. and i have no idea what is good/bad :p

So what should be done? i can't really give up on static float arrays

kinke · 2021-11-20T01:29:40Z

I think the memcmp-ability should be checked in the frontend, and only such equality expressions be forwarded to the glue layer, the other ones lowered to the __equals template => no more _adEq2 usages, probably all array equality comparisons working with -betterC, simpler glue layers for all compilers…

Edit: Pinging @ibuclaw.

ibuclaw · 2021-11-20T07:44:08Z

I think the memcmp-ability should be checked in the frontend, and only such equality expressions be forwarded to the glue layer, the other ones lowered to the __equals template => no more _adEq2 usages, probably all array equality comparisons working with -betterC, simpler glue layers for all compilers…

Edit: Pinging @ibuclaw.

Seems reasonable to me.

ryuukk · 2021-11-20T18:41:10Z

Is that an easy change to do? should this PR be closed? since it's blocking me, i would like to speed up the fix, maybe i can do it if you can point me to the right direction (if the change is relatively easy to do)

RazvanN7 · 2022-01-05T09:05:39Z

@ryuukk I would say it is not simple, but it's not extremely complicated also. Here are some pointers:

This is the part where e2ir.d checks for memcmp-ability [1]
This is the branch where adEq2 is chosen [2]

You already know where in the frontend the checks for array equality are performed (expressionsem.d).

What needs to be done is to add the proper checks in expressionsem.d as to eliminate the code in [2]. Essentially, you need to make sure that the checks in [1] are present in the frontend and, if fulfilled, the expression is forwarded to the gluelayer; everything else is lowered to __equals. Should be straightforward.

[1] https://github.com/dlang/dmd/blob/master/src/dmd/e2ir.d#L1860
[2] https://github.com/dlang/dmd/blob/master/src/dmd/e2ir.d#L1941

RazvanN7 · 2022-01-05T09:18:37Z

Oh, I just noticed that @kinke is working on this on #13331 so I'll close this. Thanks @ryuukk

ryuukk added 2 commits November 19, 2021 04:03

Fix Issue 22082 - Static float array comparison now lowered to __equals

4e12dcc

Added test

4a35a13

dlang-bot added the Bug Fix label Nov 19, 2021

ryuukk mentioned this pull request Nov 19, 2021

Improve error message for when equality expression requires TypeInfo when using -betterC #13319

Closed

ryuukk added 3 commits November 19, 2021 04:15

Cleanup trailing whitespaces

c36e0fa

Cleanup trailing whitespaces

ad85633

Also lower static arrays

65889b5

Solve this: ```D struct VertexB { float[2] pos; } struct Hold { VertexB v; } extern(C) int main() { Hold[1] h_a; Hold[1] h_b; assert(h_a != h_b); }

JohanEngelen reviewed Nov 19, 2021

View reviewed changes

Update test for other floating points

3026078

ryuukk commented Nov 19, 2021

View reviewed changes

kinke reviewed Nov 19, 2021

View reviewed changes

RazvanN7 closed this Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 22082 - Static float array comparison now lowered to __equals #13320

Fix Issue 22082 - Static float array comparison now lowered to __equals #13320

ryuukk commented Nov 19, 2021

dlang-bot commented Nov 19, 2021

JohanEngelen Nov 19, 2021

ryuukk Nov 19, 2021 •

edited

JohanEngelen Nov 19, 2021

ryuukk Nov 19, 2021

ryuukk left a comment

ryuukk Nov 19, 2021 •

edited

kinke Nov 19, 2021

ryuukk Nov 19, 2021

kinke commented Nov 19, 2021

ryuukk commented Nov 19, 2021

kinke commented Nov 20, 2021 •

edited

ibuclaw commented Nov 20, 2021

ryuukk commented Nov 20, 2021

RazvanN7 commented Jan 5, 2022 •

edited

RazvanN7 commented Jan 5, 2022

Fix Issue 22082 - Static float array comparison now lowered to __equals #13320

Fix Issue 22082 - Static float array comparison now lowered to __equals #13320

Conversation

ryuukk commented Nov 19, 2021

dlang-bot commented Nov 19, 2021

Bugzilla references

Testing this PR locally

JohanEngelen Nov 19, 2021

Choose a reason for hiding this comment

ryuukk Nov 19, 2021 • edited

Choose a reason for hiding this comment

JohanEngelen Nov 19, 2021

Choose a reason for hiding this comment

ryuukk Nov 19, 2021

Choose a reason for hiding this comment

ryuukk left a comment

Choose a reason for hiding this comment

ryuukk Nov 19, 2021 • edited

Choose a reason for hiding this comment

kinke Nov 19, 2021

Choose a reason for hiding this comment

ryuukk Nov 19, 2021

Choose a reason for hiding this comment

kinke commented Nov 19, 2021

ryuukk commented Nov 19, 2021

kinke commented Nov 20, 2021 • edited

ibuclaw commented Nov 20, 2021

ryuukk commented Nov 20, 2021

RazvanN7 commented Jan 5, 2022 • edited

RazvanN7 commented Jan 5, 2022

ryuukk Nov 19, 2021 •

edited

ryuukk Nov 19, 2021 •

edited

kinke commented Nov 20, 2021 •

edited

RazvanN7 commented Jan 5, 2022 •

edited