-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: unnecessary shift-check #48213
Comments
another example: |
Ive been investigating this second example.
at b5 it has a proof tree as follows:
It fails to utilize the intermediary values. I added have a small patch that enables the compiler to add more data to the prove DAG, its very specific ATM but could be more general: This adds checks in the SSA pass where if the first argument to a conditional is a 0 constant, and the second happens to be inverted from another variable that happened to already be compared to 0, it adds the knowlege that the inverted value it is I dont know how to actually run benchmarks in the gc, I get a bunch of "cannot access internal package" errors. |
Change https://go.dev/cl/599096 mentions this issue: |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes
What did you do?
See https://go.godbolt.org/z/o5PqscasE for code.
On line 11/12 panics when
endreg
, is bigger than 29.On line 15 it is checked that
h
is negative.On line 16 there is a branch that panics on negative
-h
.This might make sense if
h
could be -128, but it cant because of the check on line 11.What did you expect to see?
I expected the go compiler to combine these checks and to avoid the extra branch+2 instructions.
What did you see instead?
Out of the assembly I gather that the go compiler fails to link line 15 with 16.
The gc also check the same thing twice, it also fails to see that
h < 0
is the same as-h >= 0
.https://go.godbolt.org/z/hsMnMa1W1 does what I expect, and the gc manages to proof that
-h
is less than0x1f
, but more importantly also that-h
is non-negative.The text was updated successfully, but these errors were encountered: