Skip to content

Conversation

@MaxGraey
Copy link
Contributor

(i32(x) < 0) | (i32(y) < 0)   ==>   i32(x | y) < 0
(i64(x) < 0) | (i64(y) < 0)   ==>   i64(x | y) < 0

@kripken
Copy link
Member

kripken commented Nov 23, 2021

I spent some time sketching out what I mean. Hopefully this is clearer. It's not polished (I didn't check if it compiles) but this is the general shape I have in mind, PTAL

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index 2cbb28c66..d318c4a94 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -2538,35 +2538,77 @@ private:
             }
             default: {
             }
           }
         }
       }
     }
     {
       // (i32(x) != 0) | (i32(y) != 0)   ==>   i32(x | y) != 0
       // (i64(x) != 0) | (i64(y) != 0)   ==>   i64(x | y) != 0
+      Binary *bx, *by;
       Expression *x, *y;
+      Const *cx, *cy;
       if (matches(curr,
-                  binary(OrInt32,
-                         binary(Ne, any(&x), ival(0)),
-                         binary(Ne, any(&y), ival(0)))) &&
+                  binary(Or,
+                         binary(&bx, any(&x), ival(&cx)),
+                         binary(&by, any(&y), ival(&cy)))) &&
+          bx->op == by->op &&
+          ExpressionAnalyzer::equal(cx, cy) &&
+          preservesOr(left) &&
           x->type == y->type) {
         auto* inner = curr->left->cast<Binary>();
-        inner->left = Builder(*getModule())
+        bx->left = Builder(*getModule())
                         .makeBinary(Abstract::getBinary(x->type, Or), x, y);
-        return inner;
+        return bx;
       }
     }
     return nullptr;
   }
 
+  // Check whether an operation preserves the Or operation through it, that is,
+  //
+  //   F(x | y) = F(x) | F(y)
+  //
+  // Mathematically that means F is homomorphic with respect to the | operation.
+  //
+  // F(x) is seen as taking a single parameter of its first child. That is, the
+  // first child is |x|, and the rest is constant. For example, if we are given
+  // a binary with operation != and the right child is a constant 0, then
+  // F(x) = (x != 0).
+  bool preservesOr(Binary* binary) {
+    // x != 0 | y != 0    ===    (x | y) != 0
+    // This effectively checks if any bits are set in x or y.
+    if (match(binary, Ne, any(), ival(0))) {
+      return true;
+    }
+
+    // x < 0 | y < 0    ===    (x | y) < 0
+    // This effectively checks if x or y have the sign bit set.
+    if (match(binary, Lt, any(), ival(0))) {
+      return true;
+    }
+
+    // (x >>> c) | (y >>> c)    ===    (x | y) >>> c
+    // This effectively shifts and merges the bits, the order of which does not
+    // matter.
+    if (match(binary, Shr, any(), ival())) {
+      return true;
+    }
+
+    return false;
+  }
+
+  bool preservesOr(Unary* unary) {
+    .. // can add this in the future perhaps
+  }
+
   // fold constant factors into the offset
   void optimizeMemoryAccess(Expression*& ptr, Address& offset) {
     // ptr may be a const, but it isn't worth folding that in (we still have a
     // const); in fact, it's better to do the opposite for gzip purposes as well
     // as for readability.
     auto* last = ptr->dynCast<Const>();
     if (last) {
       uint64_t value64 = last->value.getInteger();
       uint64_t offset64 = offset;
       if (getModule()->memory.is64()) {

@MaxGraey
Copy link
Contributor Author

Thanks! I refactored this. Except couple of changes. The major is removing (x >>> c) | (y >>> c) ==> (x | y) >>> c rule due to it already described as part of much more general rule here: #4342

@MaxGraey
Copy link
Contributor Author

Btw isn't this should be

+  //   F(x) | F(y) -> F(x | y)

which is Inversed map of a group isomorphism
instead:

+  //   F(x | y) = F(x) | F(y)

?
Inversed map of a group isomorphism is also group of homomorphism. But I guess inverses form is more precise

return inner;
binary(&bx, any(&x), ival(&cx)),
binary(&by, any(&y), ival(&cy)))) &&
bx->op == by->op && x->type == y->type && cx->value == cy->value &&
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a test for the corner cases of cx->value != cy->value or of bx->op != by->op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done!

@MaxGraey MaxGraey requested a review from kripken December 3, 2021 15:40
@kripken kripken merged commit 80ba255 into WebAssembly:main Dec 5, 2021
@MaxGraey MaxGraey deleted the combine-or-lessthan-zeros branch December 5, 2021 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants