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

cmd/compile: bugs in mips rules #38648

Open
randall77 opened this issue Apr 24, 2020 · 4 comments
Open

cmd/compile: bugs in mips rules #38648

randall77 opened this issue Apr 24, 2020 · 4 comments

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Apr 24, 2020

I came across some bugs in the mips rules while reviewing the typed aux conversion for those rules.

Constant shifts require 0-31 as the shift amount. Rules like this:

(SLL x (MOVWconst [c])) => (SLLconst x [c])

Might violate that invariant. Might just need &31 in a few places.

We should check all the other uses of constant shifts to make sure they obey the invariant listed in the opcode definition. Maybe even define a new aux type range0to31 or something to help with enforcement (which the check pass can check).

@ALTree

@ALTree
Copy link
Member

@ALTree ALTree commented Apr 24, 2020

For the record, two incorrect rules that were deleted were:

(SLL _ (MOVWconst [c])) && uint32(c)>=32 -> (MOVWconst [0])
(SRL _ (MOVWconst [c])) && uint32(c)>=32 -> (MOVWconst [0])

Deletion occurred in CL 229637. They were never triggered when building std.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 24, 2020

I'm not sure I understand why those rules are incorrect. Maybe I missed something.

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Apr 24, 2020

To be clear, I don't think there is an actual code generation bug.

The docs for SRAconst, for example, just say "arg0 >> auxInt, signed". But I don't think that's what it actually does, at least if you interpret >> as a Go shift. I think they do arg0 >> (auxint%32). In which case, it should say that. But better would be to say that the constant shifts must have an arg 0-31. That would ensure a canonical representation for constant shifts, which might help a bit during CSE, and more generally just be clearer to understand.

That's what we do on other archs, e.g. for amd64/SHLQconst: arg0 << auxint, shift amount 0-63

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 24, 2020

Okay, thanks! That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.