Skip to content

cmd/compile: bugs in mips rules #38648

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.

Sign up for GitHub

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

Jump to bottom
Closed
randall77 opened this issue Apr 24, 2020 · 5 comments
Closed

cmd/compile: bugs in mips rules #38648

randall77 opened this issue Apr 24, 2020 · 5 comments
Labels
arch-mips compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@randall77
Copy link
Contributor

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 commented Apr 24, 2020 •
edited
Loading

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.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 24, 2020
@cherrymui
Copy link
Member

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

@randall77
Copy link
Contributor Author

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
Member

Okay, thanks! That makes sense.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@mknyszek mknyszek moved this to Triage Backlog in Go Compiler / Runtime Jul 15, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
@HeliC829
Copy link
Contributor

It seems that this PR #42587 had been resloved it .

@github-project-automation github-project-automation bot moved this from Triage Backlog to Done in Go Compiler / Runtime Apr 19, 2023
@golang golang locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-mips compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@ALTree @randall77 @gopherbot @seankhliao @cherrymui @HeliC829