Skip to content

Rewrite Partial optimization to be cleaner#4208

Merged
rhendric merged 1 commit intopurescript:masterfrom
rhendric:rhendric/rewrite-partial-optimization
Jan 30, 2022
Merged

Rewrite Partial optimization to be cleaner#4208
rhendric merged 1 commit intopurescript:masterfrom
rhendric:rhendric/rewrite-partial-optimization

Conversation

@rhendric
Copy link
Copy Markdown
Member

@rhendric rhendric commented Nov 24, 2021

This feature shrinks the generated JS code for declarations that use
empty type classes, such as Partial, but is otherwise not expected to
have user-visible consequences.

A previous issue involving Partial resulted in an ad-hoc optimization
pass added to the CoreFn phase. This pass consumed code generated by the
exhaustiveness checker, which used UnusedIdent in a questionable
manner, and ran a rewrite on it that assumed that the only part of the
compile that would use UnusedIdent in such a questionable manner was
necessarily the exhaustiveness checker.

This commit:

  • simplifies the code generated by the exhaustiveness checker for
    partial matches, in particular removing any use of UnusedIdents;

  • removes the offending CoreFn optimization pass;

  • solves the problem originally motivating said optimization pass by
    using an UnusedIdent for the generated parameter when type-checking
    a value constrained by an empty type class (such parameters,
    importantly for our sanity, are actually unused);

  • and backs out a related hack in TCO that worked around such generated
    parameters being present but called without an argument.

The intended result is simpler compiler code; happy side effects include
simpler error messages involving partial pattern matches (seen here in a
few golden test output changes) and simpler generated code (fewer unused
function parameters, fewer local variables in some TCO-triggering
functions).

This backs out #3218 (except for the test) and a small part of #3416.

For the two test files added in those PRs, here are diffs of the generated code before and after this PR:

@@ -30,7 +30,7 @@
     }
 };
 var whenAccessingSuperDict = (function () {
-    var bar = function (dictWithArgEmpty) {
+    var bar = function () {
         return function (x) {
             return x;
         };
@@ -80,8 +80,8 @@
 };
 var whenAliasEmptyClass = Check.value;
 var main = (function () {
-    var $13 = Data_Eq.eq(eqCheck)(Check.value)(whenEmpty) && (Data_Eq.eq(eqCheck)(Check.value)(whenHasEmptySuper) && (Data_Eq.eq(eqCheck)(Check.value)(whenHasNonEmptySuper) && (Data_Eq.eq(eqCheck)(Check.value)(whenAliasEmptyClass) && Data_Eq.eq(eqCheck)(Check.value)(whenAccessingSuperDict))));
-    if ($13) {
+    var $10 = Data_Eq.eq(eqCheck)(Check.value)(whenEmpty) && (Data_Eq.eq(eqCheck)(Check.value)(whenHasEmptySuper) && (Data_Eq.eq(eqCheck)(Check.value)(whenHasNonEmptySuper) && (Data_Eq.eq(eqCheck)(Check.value)(whenAliasEmptyClass) && Data_Eq.eq(eqCheck)(Check.value)(whenAccessingSuperDict))));
+    if ($10) {
         return Effect_Console.log("Done");
     };
     return Control_Applicative.pure(Effect.applicativeEffect)(Data_Unit.unit);
@@ -1,19 +1,17 @@
 "use strict";
 var Effect_Console = require("../Effect.Console/index.js");
-var partialTCO = function ($copy_dictPartial) {
+var partialTCO = function () {
     return function ($copy_v) {
         return function ($copy_v1) {
-            var $tco_var_dictPartial = $copy_dictPartial;
             var $tco_var_v = $copy_v;
             var $tco_done = false;
             var $tco_result;
-            function $tco_loop(dictPartial, v, v1) {
+            function $tco_loop(v, v1) {
                 if (v && v1 === 0) {
                     $tco_done = true;
                     return 0;
                 };
                 if (v) {
-                    $tco_var_dictPartial = undefined;
                     $tco_var_v = true;
                     $copy_v1 = v1 - 1 | 0;
                     return;
@@ -21,7 +19,7 @@
                 throw new Error("Failed pattern match at Main (line 11, column 1 - line 11, column 47): " + [ v.constructor.name, v1.constructor.name ]);
             };
             while (!$tco_done) {
-                $tco_result = $tco_loop($tco_var_dictPartial, $tco_var_v, $copy_v1);
+                $tco_result = $tco_loop($tco_var_v, $copy_v1);
             };
             return $tco_result;
         };

Checklist:

  • Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

JordanMartinez
JordanMartinez approved these changes Nov 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

LGTM as far as I can tell.

@JordanMartinez
Copy link
Copy Markdown
Contributor

JordanMartinez commented Nov 24, 2021

We should check that the entire package set still compiles with this PR.

@rhendric
Copy link
Copy Markdown
Member Author

rhendric commented Nov 24, 2021

We should check that the entire package set still compiles with this PR.

Just checked—it does. However, this is a bunch of tweaks to optimizations; compilation errors aren't the lurking bugs I would have expected. This is more likely to cause problems at run time, not that I'm anticipating any.

@JordanMartinez
Copy link
Copy Markdown
Contributor

JordanMartinez commented Nov 24, 2021

Right. But the package set compiling is just a sanity check that nothing unexpected was affected by this.

@rhendric rhendric force-pushed the rhendric/rewrite-partial-optimization branch from 7b5b3e2 to d6a6232 Compare December 21, 2021 17:46
@JordanMartinez
Copy link
Copy Markdown
Contributor

JordanMartinez commented Dec 21, 2021

Package set compiles. Any other core team members want to weigh in on this?

@rhendric
Copy link
Copy Markdown
Member Author

rhendric commented Dec 21, 2021

#4205 is going to (trivially) conflict with this; I just want to make sure that one lands first (in a few hours, per my announcement in #4206) so that my PR bears the cost of resolving the conflict.

@rhendric
Rewrite Partial optimization to be cleaner
728a681 
This feature shrinks the generated JS code for declarations that use
empty type classes, such as `Partial`, but is otherwise not expected to
have user-visible consequences.

A previous issue involving `Partial` resulted in an ad-hoc optimization
pass added to the CoreFn phase. This pass consumed code generated by the
exhaustiveness checker, which used `UnusedIdent` in a questionable
manner, and ran a rewrite on it that assumed that the only part of the
compile that would use `UnusedIdent` in such a questionable manner was
necessarily the exhaustiveness checker.

This commit:

* simplifies the code generated by the exhaustiveness checker for
  partial matches, in particular removing any use of `UnusedIdent`s;

* removes the offending CoreFn optimization pass;

* solves the problem originally motivating said optimization pass by
  using an `UnusedIdent` for the generated parameter when type-checking
  a value constrained by an empty type class (such parameters,
  importantly for our sanity, are actually unused);

* and backs out a related hack in TCO that worked around such generated
  parameters being present but called without an argument.

The intended result is simpler compiler code; happy side effects include
simpler error messages involving partial pattern matches (seen here in a
few golden test output changes) and simpler generated code (fewer unused
function parameters, fewer local variables in some TCO-triggering
functions).
@rhendric rhendric force-pushed the rhendric/rewrite-partial-optimization branch from d6a6232 to 728a681 Compare December 21, 2021 23:27
@rhendric rhendric merged commit 69955a7 into purescript:master Jan 30, 2022
@rhendric rhendric deleted the rhendric/rewrite-partial-optimization branch January 30, 2022 09:04
@JordanMartinez JordanMartinez mentioned this pull request Mar 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@JordanMartinez JordanMartinez JordanMartinez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rhendric @JordanMartinez