Skip to content
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

(Bug 3147) Reimplemented Value-Range Propagation. #116

Merged
merged 5 commits into from
Jun 27, 2011
Merged

(Bug 3147) Reimplemented Value-Range Propagation. #116

merged 5 commits into from
Jun 27, 2011

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Jun 12, 2011

This should have fixed bug 3147 (and bug 6000, bug 5225). The changes are:

  • VRP is applied on all integer arithmetic operators, i.e. + - * / % << >> >>> & | ^, and the unary ~ -.
  • Range with signed numbers is properly supported by using a 65-bit number internally.
  • The algorithm for & | ^ are rewritten to produce a much tighter range than before.
  • Several unlisted bugs producing a wrong range regarding << >> >>> are fixed.

This should have fixed Bug 3147. The changes are:
 - VRP is applied on all integer arithmetic operators, i.e. + - * / % << >> >>> & | ^, and the unary ~ -.
 - Range with signed numbers is properly supported by using a 65-bit number internally.
 - The algorithm for & | ^ are rewritten to produce a much tighter range than before.
 - Several unlisted bugs producing a wrong range regarding << >> >>> are fixed
@donc
Copy link
Collaborator

donc commented Jun 12, 2011

Awesome!

@andralex
Copy link
Member

There was this large discussion about VRP for & and |. Some people came up with pretty compelling algorithms - did you follow that? If not, you may want to compare notes.

@andralex
Copy link
Member

One more thing - is VRP effective in switch statements? Consider:

int x;
switch (x & 7) { ... }

With VRP in tow, the compiler should figure that only cases 0 through 7 are needed.

@kennytm
Copy link
Contributor Author

kennytm commented Jun 13, 2011

@andralex: Thanks for reminding. I've checked Adam's algorithm, and his provides a tighter upper bound, but a looser lower bound. So I have combined the two. This should be able to provide a tightest bound in eighty-something percent of all possible cases for |. But & and ^ are still not very good. See https://gist.github.com/1022794 for the result.

As for switch statement, VRP may not produce satisfactory result, e.g.

int x;
final switch (x & 0xf0) {
   ...
}

will require every (241) case in (0, 1, 2, ..., 0xf0) be present, although only 16 cases (0, 0x10, ..., 0xf0) are needed.

@andralex
Copy link
Member

@kennytm: Thanks, that's great. The exhaustive checker has its role and merits, but I'd generally hope for bounds that are provably the tightest. (Then the exhaustive checker would serve as a debugger.) Absent the theoretical proof, of course this pull is much better than what we have.

Regarding switch - good point. I think a common pattern is to use switch with a small power-of-two-minus-one mask, so we should make sure we get that covered.

@andralex
Copy link
Member

I'll leave it to Walter and Don to pull this.

@WalterBright
Copy link
Member

Halp! This can't be auto-merged, what do I do? People didn't like when I did it manually :-(

@CyberShadow
Copy link
Member

Do what Linus does, and tell the submitter to do the honors instead!

http://www.youtube.com/watch?v=4XpnKHJAok8#t=35m45s

@WalterBright
Copy link
Member

If Linus says so, it must be ok!

Conflicts:
	src/freebsd.mak
	src/linux.mak
	src/openbsd.mak
	src/osx.mak
	src/solaris.mak
@kennytm
Copy link
Contributor Author

kennytm commented Jun 27, 2011

@WalterBright: done :)

@WalterBright
Copy link
Member

awesome!

WalterBright added a commit that referenced this pull request Jun 27, 2011
(Bug 3147) Reimplemented Value-Range Propagation.
@WalterBright WalterBright merged commit dd2b9df into dlang:master Jun 27, 2011
braddr pushed a commit to braddr/dmd that referenced this pull request Sep 15, 2011
Fixed issue 6193: Appender.clear() functionality or documentation
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.

5 participants