Skip to content

Generalize transforms for #3153 #3193

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.

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 14 commits into from
Oct 5, 2020
Merged

Conversation

MaxGraey
Copy link
Contributor

@MaxGraey MaxGraey commented Oct 3, 2020

It's more general (additional) version of #3153 which also handle negative constant dividers:
(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 didn't use these possibilities.

cc @kripken @tlively

@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 3, 2020

Quick fuzzed:

Invocations so far:
   FuzzExec: 1771
   CompareVMs: 534
   CheckDeterminism: 173
   Wasm2JS: 419
   Asyncify: 466

ITERATION: 2115

Will fuzz it more carefully tomorrow.

@MaxGraey MaxGraey requested a review from tlively October 3, 2020 09:14
@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 3, 2020

refuzzed

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Can you remind me why it's not enough to just do x % -C_pot --> x & (abs(C_pot) - 1)? Why does the comparison with zero need to be part of the pattern?

MaxGraey and others added 3 commits October 5, 2020 01:55
Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>
@MaxGraey
Copy link
Contributor Author

MaxGraey commented Oct 4, 2020

Can you remind me why it's not enough to just do x % -C_pot --> x & (abs(C_pot) - 1)? Why does the comparison with zero need to be part of the pattern?

(signed)x % -C_pot without other constrains like comparison with zero is a bit tricky due to two complement (asymmetric) nature of integers, so for negative values we should follow path which care about overflows. That's for example how this look on VMs/LLVM:

https://godbolt.org/z/E5b3rx

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM with that one last comment addressed :)

@MaxGraey MaxGraey requested a review from tlively October 5, 2020 16:24
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Awesome, thanks :)

@tlively tlively merged commit 654cfca into WebAssembly:master Oct 5, 2020
@MaxGraey MaxGraey deleted the improve-3153 branch October 5, 2020 16:34
MaxGraey added a commit to MaxGraey/binaryen that referenced this pull request Oct 6, 2020
tlively pushed a commit that referenced this pull request Oct 6, 2020
`(signed)x % (i32|i64).min_s ==> (x & (i32|i64).max_s)` is not valid unless compared to zero.
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