Simplify signed reminders which compared with zero #3153

MaxGraey · 2020-09-21T09:43:11Z

eqz((signed)x % C_pot) -> eqz(x & (C_pot - 1))
~~(signed)x % C_pot == 0 -> x & (C_pot - 1) == 0~~
(signed)x % C_pot != 0 -> x & (C_pot - 1) != 0

src/passes/OptimizeInstructions.cpp

MaxGraey · 2020-09-21T15:13:11Z

Fuzzed:

Invocations so far:
   FuzzExec: 19686
   CompareVMs: 5406
   CheckDeterminism: 1776
   Wasm2JS: 4604
   Asyncify: 4940

ITERATION: 23155

tlively

Why do you need the eqz version of this pattern? It looks like it doesn't do anything that the binary version doesn't already do.

tlively · 2020-09-25T04:33:02Z

src/passes/OptimizeInstructions.cpp

@@ -570,6 +570,22 @@ struct OptimizeInstructions
        return ret;
      }
    } else if (auto* unary = curr->dynCast<Unary>()) {
+      if (unary->op == EqZInt32 || unary->op == EqZInt64) {


Could you move this up to the top of this function with the other matcher-based transformations? The more we can avoid putting new logic in this highly-nested part of the function, the better. It would also be good to start the match with the top-level expression, even though we already know it is a unary expression. Moving this will also let you use the binary matcher without the Match:: prefix.

I decide use slightly different approach due to input wasm could contain mixed eqz and x == 0 operations and due to x == 0 --> eqz x transform apply in early stage exactly after canonicalization I guess better just remove x == 0 matching and always check only x != 0. So x == 0 --> eqz x rule may be make sense even move to canonicalization routine.

I still think it would be good to move these up to where the other matcher-based transformations are. It's ok if you have to move other code as well to make it work out. I would also be willing to do a non-functional change to port most of this code to use matchers first if that would make it easier for you.

tlively · 2020-09-25T04:35:55Z

src/passes/OptimizeInstructions.cpp

+        using namespace Match;
+        Const* c;
+        Binary* inner;
+        Expression* left;


It looks like left is only used to get the child type of the binary expression. You should be able to save a variable by using c->type for that instead.

tlively · 2020-09-25T04:36:33Z

src/passes/OptimizeInstructions.cpp

+        // eqz((signed)x % C_pot)   ==>   eqz(x & (C_pot - 1))
+        if (matches(unary->value,
+                    Match::binary(
+                      &inner, Abstract::RemS, any(&left), constant(&c)))) {


You could use ival instead of constant to make this more compact.

It will be impossible after this I guess

src/passes/OptimizeInstructions.cpp

MaxGraey · 2020-09-25T08:31:38Z

Why do you need the eqz version of this pattern? It looks like it doesn't do anything that the binary version doesn't already do

Initially it was without eqz version but it seems it necessary. In practice I got mostly of it instead expr == 0

src/passes/OptimizeInstructions.cpp

MaxGraey · 2020-09-26T14:16:24Z

refuzzed

MaxGraey · 2020-09-29T18:22:05Z

same issue with CI as yesterday

tlively

Looks good with these final changes applied!

tlively · 2020-09-29T22:14:37Z

src/passes/OptimizeInstructions.cpp

+          if (IsPowerOf2((uint64_t)c->value.getInteger())) {
+            inner->op = Abstract::getBinary(c->type, Abstract::And);
+            c->value = c->value.sub(Literal::makeFromInt32(1, c->type));
+            return eqz;


This can just be return curr; and the eqz variable can be removed. Also you can use ival instead of constant and rename inner to bin to make the pattern more compact.

tlively · 2020-09-29T22:15:49Z

src/passes/OptimizeInstructions.cpp

+    // (signed)x % C_pot != 0   ==>  x & (C_pot - 1) != 0
+    if (matches(curr,
+                binary(Abstract::Ne,
+                       binary(&inner, Abstract::RemS, any(&left), constant(&c)),


Suggested change

binary(&inner, Abstract::RemS, any(&left), constant(&c)),

binary(&inner, Abstract::RemS, any(&left), ival()),

You actually don't need c here because it is already captured in right.

Hmm, I got regression:
https://github.com/WebAssembly/binaryen/runs/1184925427#step:15:6398

Yes, right and c are different constants. right is outer constant and c is inner for this expression:

(signed)x % c != 0 ==> x & (c - 1) != 0

where 0 is right.

So I reverted changes)

Oh right, my bad!

src/passes/OptimizeInstructions.cpp

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

tlively · 2020-09-29T23:18:01Z

src/passes/OptimizeInstructions.cpp

+      if (matches(
+            curr,
+            binary(Abstract::Ne,
+                   binary(&inner, Abstract::RemS, any(&left), constant(&c)),


constant can still be renamed ival here for compactness. In general, other than removing c, I think all the comments still apply.

But I should modify this constant's value as:

c->value = c->value.sub(Literal::makeFromInt32(1, left->type));

How I can do this without access to Const* expression?

ival(&c) should work fine. constant is for when the constant can be either an integer or a floating point constant.

I just wondering how about this approach:

int64_t c; Binary* inner; if (matches( curr, binary(Abstract::Ne, binary(&inner, Abstract::RemS, any(&left), ival(&c)), ival(0)))) { if (IsPowerOf2((uint64_t)c)) { if (left->type == Type::i64) { optimizePowerOf2URem(inner, (uint64_t)c); } else { optimizePowerOf2URem(inner, (uint32_t)c); } return curr; } }

Is it make sense?

constant is for when the constant can be either an integer or a floating point constant.

I see. Thanks!
I refactored

What about rename constant to val or xval / cval?

i32() i64() f32() f64() ival() ;; i32() | i64() fval() ;; f32() | f64() val() ;; ival() | fval()

MaxGraey · 2020-09-30T01:06:57Z

Fast refuzzing stats:

Invocations so far:
   FuzzExec: 5334
   CompareVMs: 1427
   CheckDeterminism: 447
   Wasm2JS: 1224
   Asyncify: 1323

ITERATION: 6259

tlively

Thanks, this looks great!

kripken · 2020-10-02T17:37:16Z

src/passes/OptimizeInstructions.cpp

+        if (matches(curr,
+                    unary(Abstract::EqZ,
+                          binary(&inner, Abstract::RemS, any(), ival(&c)))) &&
+            IsPowerOf2((uint64_t)c->value.getInteger())) {


why is the cast to uint64_t needed?

IsPowerOf2 allow only unsigned types but getInteger() return int64_t

Btw negative dividers like x % -4 == 0 currently rejected and not optimized which is ok but it seems it could be handle as (x & 3) == 0 as well.

Wouldn't it work without the cast, then? The function is

template<typename T> bool isPowerOf2(T v) { return v != 0 && (v & (v - 1)) == 0; }

which just cares about bits anyhow. There should be no compiler warning or error here? Maybe I'm not sure what you mean by "allow only".

I'm not sure signed type does make sense for isPowerOf2 due to negative values always return false. But you're right casting to uint64_t here unnecessary.

Logically, I agree, it's odd to ask about a negative number. But we just have bits here, really. A Literal just stores the bits, without a sign (just like wasm and LLVM IR, etc.).

I'll create PR which handle also negative dividers: x % -C_pot == 0 -> (x & (abs(C_pot) - 1)) == 0 and remove this cast as well

Implement a more general (additional) version of #3153 which also handles negative constant divisors: `(int32)x % -4 == 0` --> `(x & 3) == 0` `x % -C_pot == 0` --> `(x & (abs(C_pot) - 1)) == 0` and special two-complement values as well: `(int32)x % 0x80000000 == 0` --> `(x & 0x7fffffff) == 0` `(int64)x % 0x8000000000000000 == 0` --> `(x & 0x7fffffffffffffff) == 0` as separete rules: `(int32)x % 0x80000000` --> `x & 0x7fffffff` `(int64)x % 0x8000000000000000` --> `x & 0x7fffffffffffffff` The [previous pr](#3153) didn't use these possibilities.

Simplify signed reminders which compared with zero

31a5276

MaxGraey commented Sep 21, 2020

View reviewed changes

src/passes/OptimizeInstructions.cpp Outdated Show resolved Hide resolved

add match for unary. More tests

25aca33

MaxGraey commented Sep 21, 2020

View reviewed changes

src/passes/OptimizeInstructions.cpp Outdated Show resolved Hide resolved

more test

8654640

tlively reviewed Sep 25, 2020

View reviewed changes

MaxGraey added 4 commits September 25, 2020 11:39

Merge branch 'master' into opt-rem_s-w-cmp-zero

c934b6f

remove expr == 0 matching

25c5658

refactor

5601d17

remove redundancy and refactoring

0adb236

MaxGraey commented Sep 25, 2020

View reviewed changes

src/passes/OptimizeInstructions.cpp Outdated Show resolved Hide resolved

MaxGraey requested a review from tlively September 26, 2020 12:45

Merge branch 'master' into opt-rem_s-w-cmp-zero

969b642

MaxGraey added 2 commits September 29, 2020 16:52

Merge branch 'master' into opt-rem_s-w-cmp-zero

9ba1540

update fixtures

dd6099a

MaxGraey added 4 commits September 30, 2020 00:56

move eqz rule on top

d2cee70

trigger ci

99fe2a6

Merge branch 'master' into opt-rem_s-w-cmp-zero

12bec6f

update fixtures after sync with master

99df9be

tlively reviewed Sep 29, 2020

View reviewed changes

MaxGraey and others added 6 commits September 30, 2020 01:37

Update src/passes/OptimizeInstructions.cpp

a881c38

Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>

refactor

2e41728

more

998ee26

more (scope)

cc4bc4e

refactor according review

f583965

space

4a01171

revert changes

6bb1aa5

tlively reviewed Sep 29, 2020

View reviewed changes

MaxGraey added 4 commits September 30, 2020 02:57

refactor

1ca1bc6

same for eqz rule

e8cc01b

more

c9fc299

use c->type instead left->type which simplify expr != 0 rule

002906a

simplify eqz rule as well

4bd7810

MaxGraey requested a review from tlively September 30, 2020 01:10

tlively approved these changes Sep 30, 2020

View reviewed changes

tlively merged commit 7e99538 into WebAssembly:master Sep 30, 2020

MaxGraey deleted the opt-rem_s-w-cmp-zero branch September 30, 2020 08:20

kripken reviewed Oct 2, 2020

View reviewed changes

MaxGraey mentioned this pull request Oct 3, 2020

Generalize transforms for #3153 #3193

Merged

	binary(&inner, Abstract::RemS, any(&left), constant(&c)),
	binary(&inner, Abstract::RemS, any(&left), ival()),

Simplify signed reminders which compared with zero #3153

Simplify signed reminders which compared with zero #3153

Uh oh!

Conversation

MaxGraey commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxGraey commented Sep 21, 2020

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGraey Sep 25, 2020 • 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!

Uh oh!

MaxGraey commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MaxGraey commented Sep 26, 2020

Uh oh!

MaxGraey commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively left a comment

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!

This comment was marked as outdated.

Uh oh!

MaxGraey Sep 29, 2020 • 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!

MaxGraey Sep 29, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGraey Sep 29, 2020 • 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!

MaxGraey Sep 29, 2020 • 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!

MaxGraey Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

MaxGraey commented Sep 21, 2020 •

edited

Loading

MaxGraey Sep 25, 2020 •

edited

Loading

MaxGraey commented Sep 25, 2020 •

edited

Loading

MaxGraey commented Sep 29, 2020 •

edited

Loading

MaxGraey Sep 29, 2020 •

edited

Loading

MaxGraey Sep 29, 2020 •

edited

Loading

MaxGraey Sep 29, 2020 •

edited

Loading

MaxGraey Sep 29, 2020 •

edited

Loading

MaxGraey Sep 30, 2020 •

edited

Loading

kripken Oct 2, 2020 •

edited

Loading

MaxGraey Oct 2, 2020 •

edited

Loading

MaxGraey Oct 2, 2020 •

edited

Loading

kripken Oct 2, 2020 •

edited

Loading

MaxGraey Oct 2, 2020 •

edited

Loading