Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement more cases for getMaxBits #2879
Implement more cases for getMaxBits #2879
Changes from 73 commits
ee08839
374c9b6
3a0f72e
091e15e
18a2aa2
168fed4
d04ae7d
c08376c
2dc792c
56146d3
3f68768
36f73e5
c77d5d4
d60b53e
0bba5b6
f43c8db
a6415e1
d979620
6685e6d
7726f18
a39091a
908eb96
df767f4
73ad57d
6b8d232
f387148
38ee4e7
74f10d5
fa13337
2e9ab76
3ea4530
252ce01
fd4e8cf
60b7b55
d93f65d
c9fe2cb
14ae90c
90f39d4
5a99214
64aded9
4b2061a
216b6fa
c631b94
d0a4938
543d239
5673101
ae827a8
8567487
957e945
d5d6d2b
709b8ed
569362f
9545f9e
9085d8e
422e46f
551a675
d7335a8
3a27ca3
e0739c3
74e568c
ae75346
6883ce7
0cc2e85
acd6518
df40a08
da177ae
d84f9ba
5c1d1c4
f319270
55d9824
f924008
790a05c
30b1842
341a3ef
b91ee8c
719d8c3
146c966
a74f10e
b4d614d
9b2fe4d
162c94d
8404d91
4be0956
ec62420
5fc687a
0be9076
33c9eab
277d159
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed that there should not be such checks for 0 (because 0 should be handled elsewhere already)? Please remove them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove that check:
auto t = getMaxBits(&b)
t
without early check return4294967293
or-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this almost duplicate code from line 144? Why is it duplicated, and why is it different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, could you made link to this line? line 144 contain something non-similar to that code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the line that computes the max bits for a Const,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So you suggest change it to:
or
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
-1
? It's weird to seefoo
on both sides, but with a difference, is odd.I think the meaning is different than
max bits
, but I'm not sure what it is? Is it the topmost bit maybe? Or one past that?But given the below comment, and this is just for consts, you probably don't want
localInfoProvider
there - it doesn't help.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be
maxLeftBits - rightBits + 1
. It's easily check on next example:maxBits(0x7FFFFFFF / 3)
==30
maxLeftBits = maxBits(0x7FFFFFFF)
is31
rightBits = maxBits(3)
is2
31 - 2
==>29
31 - 2 + 1
==>30
also it properly handle case when
maxLeftBits == rightBits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type here looks wrong. the maxBits are both unsigned, so the difference is unsigned too. So if it would be negative, it will be a very big positive number, and "win" in the max operation.
Aside from the type it also looks wrong. We know that
(from the definition of maxBits). but that doesn't imply
The only inequality I can almost see how to prove is
but even that isn't quite right, as
right
may be 0, even ifmaxBitsRight > 0
. This is similar to the issue from before: maxBits is an upper bound.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Perhaps it's not so simple. Decide check how do this in souper and it's look more complex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems there uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easily could be rejected due to current implementation do max bits div calcs only for
x / C(onst)
: https://github.com/WebAssembly/binaryen/pull/2879/files#diff-58b7cf13906e9b5a4ba6306aaea394aaR158. Right value always const in my scenario. I don't try calc bits for general case likex / y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Yes, if the right value is a const, then this is simpler. In that case, the variable name
maxBitsRight
is confusing - it's not the max bits, it's the actual number of bits.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored and now using
bitsRight
insteadmaxBitsRight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there reason to believe that checking the right side first will save more work than checking the left side first, or is it an arbitrary choice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, right side could be cheaper due to canonization which always force constant on the right. Also
x ^ -1
is quite often case.