Array Compare Lowering by somzzz · Pull Request #6597 · dlang/dmd

somzzz · 2017-03-07T12:11:22Z

This is the dmd code which calls the new __cmp template from druntime,.
dlang/druntime#1781

UplinkCoder · 2017-03-07T12:20:39Z

src/ddmd/expression.d

                semanticTypeInfo(sc, t1.nextOf());
+
+                // Lowering to template call
+                Identifier id = Identifier.idPool("__cmp");


call it __cmp or something like that since that is the id it represents.
Also maybe it would be useful to add it to idgen.d
Such that you don't have to call idPool every time, which is not a cheap call.

andralex · 2017-03-07T17:25:34Z

OK, so this should work with the latest update to dlang/druntime#1787, could you please patch that and make sure it works?

andralex · 2017-03-08T00:28:08Z

I merged dlang/druntime#1787 so let's see what the autotester says.

andralex

LGTM of course modulo it passing tests. @WalterBright ?

andralex · 2017-03-08T00:32:04Z

src/ddmd/expression.d

            {
                semanticTypeInfo(sc, t1.nextOf());
+
+                // Lowering to template call


Would be nice to document what the template call is (it's not obvious from the sequence of AST node creations and links).

<ihaveadream>
Would be great to have the ability to create an AST fragment from a string source, a la:

auto node = makeExpression!"__cmp($1, $2)"(e1, e2);

whereby makeExpression parses the string during compilation (of dmd), creates the appropriate AST fragment, places the two expressions in lieu of $1 and $2, and returns the created expression node.

I assume a large hunk of compiler code would be drastically simplified.
</ihaveadream>

Yep, just replace the comment by // Lower to .object.__cmp(e1, e2) or something like that.

WalterBright · 2017-03-08T01:32:24Z

src/ddmd/expression.d

            if ((t1.ty == Tarray || t1.ty == Tsarray) && (t2.ty == Tarray || t2.ty == Tsarray))
            {
                semanticTypeInfo(sc, t1.nextOf());
+


Don't need TypeInfo anymore, so don't need the above line.

WalterBright · 2017-03-08T01:33:16Z

src/ddmd/expression.d

+                arguments.push(e2);
+
+                al = new CallExp(loc, al, arguments);
+                al = al.semantic(sc);


only need one call to semantic() at the end.

WalterBright · 2017-03-08T01:36:24Z

src/ddmd/expression.d

+                al = new CallExp(loc, al, arguments);
+                al = al.semantic(sc);
+
+                al = new CmpExp(op, loc, al, new IntegerExp(loc, 0, Type.tint32));


new IntegerExp(0) is sufficient.

andralex · 2017-03-08T01:59:04Z

@somzzz please take a look at the autotester (e.g. https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=2431115&isPull=true), looks like a clerical issue:

Error - file 'ddmd/expression.d' contains trailing whitespace at line 15428

All: I'm also seeing a bazillion of these: ddmd/backend/ptrntab.c:5773: warning: dereferencing type-punned pointer will break strict-aliasing rules.

somzzz · 2017-03-08T09:48:13Z

@andralex I don't see the trailing whitespace on that line.

Also, it's not clear what the problem with ptrntab.c is since there are no changes in that file.

However it seems like there are issues with dmd tests, I'll look into them. ~~It will probably require fixing __cmp.~~

somzzz · 2017-03-08T10:14:48Z

src/ddmd/expression.d

+
+                al = new CallExp(loc, al, arguments);
+                al = new CmpExp(op, loc, al, new IntegerExp(0));
+                al = al.semantic(sc);


If I comment this line, test fail_compilation/ice11755.d should pass as the output is the one desired. However, druntime tests will fail.
Calling semantic generates new output which doesn't match the one expected. What is the best approach in this case?

If I return al at the end of this code block, I get the following compilation error output for that test:

fail_comp.d(4): Error: floating point operator '!<>=' always returns false for non-floating comparisons fail_comp.d(5): Error: use '==' for non-floating comparisons rather than floating point operator '!<>' fail_comp.d(6): Error: use '!=' for non-floating comparisons rather than floating point operator '<>' fail_comp.d(7): Error: floating point operator '<>=' always returns true for non-floating comparisons fail_comp.d(8): Error: use '<=' for non-floating comparisons rather than floating point operator '!>' fail_comp.d(9): Error: use '<' for non-floating comparisons rather than floating point operator '!>=' fail_comp.d(10): Error: use '>=' for non-floating comparisons rather than floating point operator '!<' fail_comp.d(11): Error: use '>' for non-floating comparisons rather than floating point operator '!<=' fail_comp.d(12): Error: floating point operator '<>=' always returns true for non-floating comparisons fail_comp.d(13): Error: floating point operator '!<>=' always returns false for non-floating comparisons

There is no more duplication, but the output is not precisely the same as the one expected, which is

fail_compilation/ice11755.d(20): Error: '!<>=' is not defined for array comparisons fail_compilation/ice11755.d(21): Error: use '==' for non-floating comparisons rather than floating point operator '!<>' fail_compilation/ice11755.d(22): Error: use '!=' for non-floating comparisons rather than floating point operator '<>' fail_compilation/ice11755.d(23): Error: '<>=' is not defined for array comparisons fail_compilation/ice11755.d(24): Error: use '<=' for non-floating comparisons rather than floating point operator '!>' fail_compilation/ice11755.d(25): Error: use '<' for non-floating comparisons rather than floating point operator '!>=' fail_compilation/ice11755.d(26): Error: use '>=' for non-floating comparisons rather than floating point operator '!<' fail_compilation/ice11755.d(27): Error: use '>' for non-floating comparisons rather than floating point operator '!<=' fail_compilation/ice11755.d(28): Error: floating point operator '<>=' always returns true for non-floating comparisons fail_compilation/ice11755.d(29): Error: floating point operator '!<>=' always returns false for non-floating comparisons

It seems like a specific error message Error: '!<>=' is not defined for array comparisons is replaced with Error: floating point operator '!<>=' always returns false for non-floating comparisons

Calling semantic at the very end of the function was the answer to all problems.

UplinkCoder · 2017-03-08T11:39:25Z

@somzzz
try https://gist.github.com/UplinkCoder/2a8fd659eef9f3a2fdf8032f700bde80
dmd -run fixwhitespace.d src/ddmd/expression.d

somzzz · 2017-03-08T12:38:50Z

@UplinkCoder Neat! I checked it out. Luckily there was no whitespace to be found in expression.d.

andralex · 2017-03-08T13:05:28Z

Whoa, things are passing. Neat!

CyberShadow · 2017-03-08T13:09:33Z

I thought we agreed to check Martin's project tester before merging this to master...

I don't see it in the CI status listing. @MartinNowak ?

andralex · 2017-03-08T14:14:26Z

Yah, I was surprised too that setting the auto-merge label didn't wait for the CI to finish.

andralex · 2017-03-08T14:15:34Z

src/ddmd/expression.d

+        if (arrayLowering)
+        {
+            arrayLowering = arrayLowering.semantic(sc);
+            return arrayLowering;


Looks a bit pedestrian, nothing wrong with return arrayLowering.semantic(sc);

... in the future :)

This style in dmd comes from Walter wanting to be able to trivially insert prints before the return, and I tend to agree.

JackStouffer · 2017-03-08T14:47:34Z

@andralex The auto-merge label currently doesn't check the project tester. It only looks at the auto-tester, the doc builder, Circle, and CodeCov.

CyberShadow · 2017-03-08T17:20:48Z

@MartinNowak What's the status of the project tester, can we see whether this broke anything?

array compare lowering

4f00cab

UplinkCoder reviewed Mar 7, 2017

View reviewed changes

somzzz mentioned this pull request Mar 7, 2017

One more pass through __cmp dlang/druntime#1787

Merged

Idgen for __cmp

24ca016

andralex assigned WalterBright Mar 8, 2017

andralex approved these changes Mar 8, 2017

View reviewed changes

WalterBright reviewed Mar 8, 2017

View reviewed changes

Fixed @comments

b45ac54

somzzz commented Mar 8, 2017

View reviewed changes

fix failing test ice11755

a7f77f3

andralex added the Merge:auto-merge label Mar 8, 2017

dlang-bot merged commit c87ed3d into dlang:master Mar 8, 2017

andralex reviewed Mar 8, 2017

View reviewed changes

PetarKirov mentioned this pull request Mar 28, 2017

Implement optimization: compare slices using memcmp if valid ldc-developers/ldc#2047

Merged

Uh oh!

Conversation

somzzz commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Mar 7, 2017

Uh oh!

andralex commented Mar 8, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

andralex Mar 8, 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!

andralex commented Mar 8, 2017

Uh oh!

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

Uh oh!

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!

UplinkCoder commented Mar 8, 2017

Uh oh!

somzzz commented Mar 8, 2017

Uh oh!

andralex commented Mar 8, 2017

Uh oh!

CyberShadow commented Mar 8, 2017

Uh oh!

andralex commented Mar 8, 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!

JackStouffer commented Mar 8, 2017

Uh oh!

CyberShadow commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

andralex Mar 8, 2017 •

edited

Loading

somzzz commented Mar 8, 2017 •

edited

Loading