Skip to content

Heuristic for pattern matching untagged variants. #7128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 26, 2024
Merged

Heuristic for pattern matching untagged variants. #7128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c825694
Heuristic for pattern matching untagged variants.
cristianoc Oct 25, 2024
693fedf
Heuristic: size of generated code.
cristianoc Oct 25, 2024
fc6f9e9
Update CHANGELOG.md
cristianoc Oct 25, 2024
6913da9
Bring ad-hoc simplification of && generation up to speed.
cristianoc Oct 26, 2024
b12f189
respect declaration order
cristianoc Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions compiler/ml/ast_untagged_variants.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,12 +458,22 @@ module DynamicChecks = struct
Ext_list.fold_right others is_literal_1 (fun literal_n acc ->
is_literal_case literal_n ||| acc))
in
match block_cases with
| [c] -> is_not_block_case c
| c1 :: (_ :: _ as rest) ->
is_not_block_case c1
&&& is_a_literal_case ~literal_cases ~block_cases:rest e
| [] -> assert false
let list_literal_cases = true in
if list_literal_cases then
let rec mk cases =
match cases with
| [case] -> is_literal_case case
| case :: rest -> is_literal_case case ||| mk rest
| [] -> assert false
in
mk literal_cases
else
match block_cases with
| [c] -> is_not_block_case c
| c1 :: (_ :: _ as rest) ->
is_not_block_case c1
&&& is_a_literal_case ~literal_cases ~block_cases:rest e
| [] -> assert false

let is_int_tag ?(has_null_undefined_other = (false, false, false)) (e : _ t) :
_ t =
Expand Down
50 changes: 25 additions & 25 deletions tests/tests/src/UntaggedVariants.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as Primitive_array from "rescript/lib/es6/Primitive_array.js";
import * as Primitive_option from "rescript/lib/es6/Primitive_option.js";

function classify(x) {
if (x === "A" && typeof x !== "number") {
if (x === "A") {
return "A";
} else if (typeof x === "number") {
return "An integer";
Expand All @@ -24,7 +24,7 @@ function classify2(x) {
}

function cls(x) {
if (typeof x !== "object") {
if (x === "Two" || x === "One") {
if (x === "One") {
return "one";
} else {
Expand All @@ -40,13 +40,13 @@ let ListWithTuples = {};
let ListWithObjects = {};

function tuplesToObjects(l) {
if (Array.isArray(l)) {
if (l === undefined) {
return null;
} else {
return {
hd: l[0],
tl: tuplesToObjects(l[1])
};
} else {
return null;
}
}

Expand All @@ -68,7 +68,7 @@ console.log("l1", l1);
console.log("l2", l2);

function isTrue(x) {
if (typeof x !== "object") {
if (x === true) {
return true;
} else {
return x.flag;
Expand All @@ -80,7 +80,7 @@ let Truthy = {
};

function classify$1(x) {
if (x === null || typeof x !== "object") {
if (x === undefined || x === null) {
if (x === null) {
return "null";
} else {
Expand All @@ -96,7 +96,7 @@ let TwoObjects = {
};

function classify$2(x) {
if (x === "A" || x === "B") {
if (x === "B" || x === "A") {
if (x === "A") {
return "a";
} else {
Expand All @@ -112,7 +112,7 @@ let Unknown = {
};

function classify$3(x) {
if (typeof x !== "object" && typeof x !== "number" && (x === "C" || x === "B" || x === "A" || x === "D")) {
if (x === "D" || x === "C" || x === "B" || x === "A") {
switch (x) {
case "A" :
return "a";
Expand Down Expand Up @@ -173,7 +173,7 @@ let WithArray = {
};

function classify$6(x) {
if (!Array.isArray(x) && (x === null || typeof x !== "object") && typeof x !== "number" && typeof x !== "string") {
if (x === null || x === true || x === false) {
switch (x) {
case false :
return "JSONFalse";
Expand Down Expand Up @@ -214,18 +214,18 @@ let Json = {
};

function check(s, y) {
if (!Array.isArray(s)) {
if (s === "B") {
return 42;
}
let x = s[0];
if (!Array.isArray(x)) {
if (x === "B") {
return 42;
}
let tmp = s[1];
if (Array.isArray(tmp) || x === y) {
return 42;
} else {
if (tmp === "B" && x !== y) {
return 41;
} else {
return 42;
}
}

Expand All @@ -234,7 +234,7 @@ let TrickyNested = {
};

function checkEnum(e) {
if (!(e === "Two" || e === "One" || e === "Three")) {
if (!(e === "Three" || e === "Two" || e === "One")) {
return "Something else..." + e;
}
switch (e) {
Expand All @@ -252,7 +252,7 @@ let OverlapString = {
};

function checkEnum$1(e) {
if (!(e === "Two" || e === 1.0 || e === "Three")) {
if (!(e === "Three" || e === "Two" || e === 1.0)) {
return "Something else...";
}
switch (e) {
Expand All @@ -270,7 +270,7 @@ let OverlapNumber = {
};

function checkEnum$2(e) {
if (!(e === null || typeof e !== "object")) {
if (!(e === "Three" || e === "Two" || e === null)) {
return "Object...";
}
switch (e) {
Expand Down Expand Up @@ -376,7 +376,7 @@ let TestFunctionCase = {
let someJson = '[{"name": "Haan"}, {"name": "Mr"}, false]';

function check$1(s) {
if (!Array.isArray(s) && (s === null || typeof s !== "object") && typeof s !== "number" && typeof s !== "string") {
if (s === null || s === true || s === false || s === undefined) {
console.log("Nope...");
return;
}
Expand All @@ -386,11 +386,11 @@ function check$1(s) {
return;
}
let match = s[0];
if (match === true) {
if ((match === null || match === true || match === false || match === undefined) && match === true) {
Copy link
Member

Choose a reason for hiding this comment

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

@cristianoc Thanks a lot for looking into this so quickly!
Most cases look better now. But this one still feels a bit weird as the lhs of the && is obviously unnecessary.
Is it possible to improve it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and it's not related to this PR.
You're asking for a back end code simplification.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then I think this PR is ready to go and we can track the above case in a different issue. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with_literal_cases:

(e == Null) || (e ==true) || (e == false) || (e == Undefined)

without_literal_cases:

!(e instanceof Array) && ((e == Null) || (typeof e != Object)) && (typeof e != Int) && (typeof e != String)

So it is true that it generates the smaller expression (the former).
The second one must be simplified in the back-end, and not the first one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added extra simplification directly to this PR.

let match$1 = s[1];
if (match$1 === false) {
if ((match$1 === null || match$1 === true || match$1 === false || match$1 === undefined) && match$1 === false) {
let match$2 = s[2];
if (!Array.isArray(match$2) && (match$2 === null || typeof match$2 !== "object") && typeof match$2 !== "number" && typeof match$2 !== "string") {
if (match$2 === null || match$2 === true || match$2 === false || match$2 === undefined) {
console.log("Nope...");
return;
}
Expand All @@ -400,13 +400,13 @@ function check$1(s) {
return;
}
let match$3 = match$2[0];
if (!Array.isArray(match$3) && (match$3 === null || typeof match$3 !== "object") && typeof match$3 !== "number" && typeof match$3 !== "string") {
if (match$3 === null || match$3 === true || match$3 === false || match$3 === undefined) {
console.log("Nope...");
return;
}
if (typeof match$3 === "string" && match$3 === "My name is") {
let match$4 = match$2[1];
if (!Array.isArray(match$4) && (match$4 === null || typeof match$4 !== "object") && typeof match$4 !== "number" && typeof match$4 !== "string") {
if (match$4 === null || match$4 === true || match$4 === false || match$4 === undefined) {
console.log("Nope...");
return;
}
Expand Down Expand Up @@ -476,7 +476,7 @@ let PromiseSync = {
};

async function classify$10(a) {
if (typeof a !== "object" && !(a instanceof Promise) && (a === "test" || a === 12) && !Array.isArray(a)) {
if (a === 12 || a === "test") {
if (a === "test") {
console.log("testing");
return;
Expand Down
10 changes: 5 additions & 5 deletions tests/tests/src/core/Core_JsonTests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@ import * as Test from "./Test.mjs";
function decodeJsonTest() {
let json = {"someProp":{"otherProp": null, "thirdProp": [true, false]}};
let decodedCorrectly;
if (!Array.isArray(json) && (json === null || typeof json !== "object") && typeof json !== "number" && typeof json !== "string" && typeof json !== "boolean" || !(typeof json === "object" && !Array.isArray(json))) {
if (json === null || !(typeof json === "object" && !Array.isArray(json))) {
decodedCorrectly = false;
} else {
let match = json["someProp"];
if (match !== undefined && !(!Array.isArray(match) && (match === null || typeof match !== "object") && typeof match !== "number" && typeof match !== "string" && typeof match !== "boolean" || !(typeof match === "object" && !Array.isArray(match)))) {
if (match !== undefined && !(match === null || !(typeof match === "object" && !Array.isArray(match)))) {
let match$1 = match["thirdProp"];
if (match$1 !== undefined && !(!Array.isArray(match$1) && (match$1 === null || typeof match$1 !== "object") && typeof match$1 !== "number" && typeof match$1 !== "string" && typeof match$1 !== "boolean" || !(Array.isArray(match$1) && match$1.length === 2))) {
if (match$1 !== undefined && !(match$1 === null || !(Array.isArray(match$1) && match$1.length === 2))) {
let match$2 = match$1[0];
if (!Array.isArray(match$2) && (match$2 === null || typeof match$2 !== "object") && typeof match$2 !== "number" && typeof match$2 !== "string" && typeof match$2 !== "boolean" || !(typeof match$2 === "boolean" && match$2)) {
if (match$2 === null || !(typeof match$2 === "boolean" && match$2)) {
decodedCorrectly = false;
} else {
let match$3 = match$1[1];
decodedCorrectly = !Array.isArray(match$3) && (match$3 === null || typeof match$3 !== "object") && typeof match$3 !== "number" && typeof match$3 !== "string" && typeof match$3 !== "boolean" || !(typeof match$3 === "boolean" && !match$3) ? false : true;
decodedCorrectly = match$3 === null || !(typeof match$3 === "boolean" && !match$3) ? false : true;
}
} else {
decodedCorrectly = false;
Expand Down
6 changes: 3 additions & 3 deletions tests/tests/src/core/Core_NullableTests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ function shouldHandleNullableValues() {
let tUndefined = undefined;
let tValue = "hello";
let tmp;
tmp = (tNull === null || tNull === undefined) && tNull === null ? true : false;
tmp = (tNull === undefined || tNull === null) && tNull === null ? true : false;
Test.run([
[
"Core_NullableTests.res",
Expand All @@ -18,7 +18,7 @@ function shouldHandleNullableValues() {
"Should handle null"
], tmp, (prim0, prim1) => prim0 === prim1, true);
let tmp$1;
tmp$1 = (tUndefined === null || tUndefined === undefined) && tUndefined !== null ? true : false;
tmp$1 = (tUndefined === undefined || tUndefined === null) && tUndefined !== null ? true : false;
Test.run([
[
"Core_NullableTests.res",
Expand All @@ -29,7 +29,7 @@ function shouldHandleNullableValues() {
"Should handle undefined"
], tmp$1, (prim0, prim1) => prim0 === prim1, true);
let tmp$2;
tmp$2 = tValue === null || tValue === undefined || tValue !== "hello" ? false : true;
tmp$2 = tValue === undefined || tValue === null || tValue !== "hello" ? false : true;
Test.run([
[
"Core_NullableTests.res",
Expand Down
22 changes: 22 additions & 0 deletions tests/tests/src/untagged_variants_heuristic.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Generated by ReScript, PLEASE EDIT WITH CARE


function f(x) {
if (x !== null) {
return;
}
console.log("abc");
}

function ff(x) {
if (x === "G" || x === "F" || x === "E" || x === "D" || x === "C" || x === "B" || x === "A") {
Copy link
Collaborator Author

@cristianoc cristianoc Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@cknitt see this case that is now worse

This is why I propose a heuristic based on how many cases with, and without payloads exist.
To determine which code to generate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cometkim do you have a suggestion on how to set the threshold to maximise things like perf/code size etc?
There are these 2 ways of generating code. One is clearly better with a lot of cases without payload (ff here). The other is better when there are very few cases without payload and many cases with payload (e.g. json). The question is how to decide in the middle.

Copy link
Member

Choose a reason for hiding this comment

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

@cknitt see this case that is now worse

This is why I propose a heuristic based on how many cases with, and without payloads exist. To determine which code to generate.

Yes, sounds good to me, a heuristic-based approach will definitely improve the current situation without making things worse like in the example here. 👍

Copy link
Member

Choose a reason for hiding this comment

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

I think this improves the predictability because the output is easier to understand. Untagged variants are not generally expected to be the best performance.

However, from a perf perspective, typeof checks are always slightly better than full literal checks, especially if it allows you to skip other branches.

I value how much more understandable it is rather than the actual code size. If the number of circuits is the same or similar, there is no real difference. It's just that typeof checks tend to be harder to read and understand when they get complex.

There are these 2 ways of generating code. One is clearly better with a lot of cases without payload (ff here). The other is better when there are very few cases without payload and many cases with payload (e.g. json). The question is how to decide in the middle.

Not sure if possible, I'd guess it would be nice to use the current on the default strategy but do the opposite if the check matches the _ pattern.

return;
}
console.log("abc");
}

export {
f,
ff,
}
/* No side effect */
22 changes: 22 additions & 0 deletions tests/tests/src/untagged_variants_heuristic.res
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
let f = (x: JSON.t) =>
switch x {
| Null => Console.log("abc")
| _ => ()
}

@unboxed
type t =
| A
| B
| C
| D
| E
| F
| G
| Int(int)

let ff = (x: t) =>
switch x {
| Int(_) => Console.log("abc")
| _ => ()
}
14 changes: 7 additions & 7 deletions tests/tests/src/variantsMatching.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,17 +224,17 @@ function isUndefined$1(x) {
}

function plus$2(x, y) {
if (x === null || x === undefined) {
if (x === undefined || x === null) {
return y;
} else if (y === null || y === undefined) {
} else if (y === undefined || y === null) {
return x;
} else {
return x + y | 0;
}
}

function kind(x) {
if (x === null || x === undefined) {
if (x === undefined || x === null) {
if (x === null) {
return "null";
} else {
Expand Down Expand Up @@ -274,21 +274,21 @@ function isWhyNot(x) {
}

function plus$3(x, y) {
if (x === undefined || x === null || x === "WhyNotAnotherOne") {
if (x === "WhyNotAnotherOne" || x === undefined || x === null) {
switch (x) {
case null :
case undefined :
return y;
case "WhyNotAnotherOne" :
break;
}
} else if (!(y === undefined || y === null || y === "WhyNotAnotherOne")) {
} else if (!(y === "WhyNotAnotherOne" || y === undefined || y === null)) {
return {
x: x.x + y.x,
y: x.y + y.y
};
}
if (!(y === undefined || y === null || y === "WhyNotAnotherOne")) {
if (!(y === "WhyNotAnotherOne" || y === undefined || y === null)) {
return "WhyNotAnotherOne";
}
switch (y) {
Expand All @@ -301,7 +301,7 @@ function plus$3(x, y) {
}

function kind$1(x) {
if (!(x === undefined || x === null || x === "WhyNotAnotherOne")) {
if (!(x === "WhyNotAnotherOne" || x === undefined || x === null)) {
return "present";
}
switch (x) {
Expand Down
Loading