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: clean more up after ssa.Type -> types.Type refactoring #20304
Comments
Adding HelpWanted since this is some low hanging fruit that folks interested in stepping into the compiler could easily help with. |
I’d like to have a try at this - your “low hanging fruit” remark gives me some confidence. |
Go for it! Bear in mind that the tree is closed until announced on golang-dev, probably Feb 1. Since these should involve no functional changes, you might find toolstash-check to be helpful. Err on the side of smaller changes rather than one big change; it eases reviewing. If you have any questions, feel free to ask here. |
My investigations so far show me that there are 2 main parts to this - 1) Remove the duplicate type stuff from ssa, which should be “relatively” straightforward 2) Remove the dependencies between types and gc, which seems much less straightforward.
I’ve documented my analysis of the the second part - at the risk of appearing “needy”, could I ask for someone to have look at this document lest I stray too far from the desired path?
Thanks
Chris
… On 21 Jan 2018, at 4:22 pm, Josh Bleecher Snyder ***@***.***> wrote:
Go for it!
Bear in mind that the tree is closed until announced on golang-dev, probably Feb 1 <https://github.com/golang/go/wiki/Go-Release-Cycle>. Since these should involve no functional changes, you might find toolstash-check <https://github.com/mdempsky/toolstash-check> to be helpful. Err on the side of smaller changes rather than one big change; it eases reviewing. If you have any questions, feel free to ask here.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#20304 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeohTl3oxGWAhD_NjuQoVN6o6R9IN_n3ks5tMsmzgaJpZM4NV_Ht>.
|
Happy to look. (Expect possible delays from me; my schedule is quite fragmented.) |
Thanks Josh, when you have some time.
Another question - I’ve been reading sea code. If the intention is to replace the sea type definitions Bool, Int8 etc with TBOOL, TINT8 …, won’t that mean a huge volume of changes? Is it worth it?
… On 30 Jan 2018, at 4:55 pm, Josh Bleecher Snyder ***@***.***> wrote:
Happy to look. (Expect possible delays from me; my schedule is quite fragmented.)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#20304 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeohTrWwIr5Fxpdh9vA3MgQznzC3gQB_ks5tPq7AgaJpZM4NV_Ht>.
|
Did you mean to attach or link to a document? I don't see it. It's probably not worth doing a bunch of mass renames, unless it will bring a significant readability improvement. We have done significant renames before, and will surely do some again, when they matter. We've never formalized how it works, but if you think we should do some renames, I'd suggest something like:
That's probably more than you wanted to know, but hope it is helpful. :) |
I’ve enclosed a slightly revised version of the document - hopefully you can see it.
The aim is to clear up any misconceptions that I might have about this issue.
Regards
… On 2 Feb 2018, at 6:00 am, Josh Bleecher Snyder ***@***.***> wrote:
Did you mean to attach or link to a document? I don't see it.
It's probably not worth doing a bunch of mass renames, unless it will bring a significant readability improvement. We have done significant renames before, and will surely do some again, when they matter. We've never formalized how it works, but if you think we should do some renames, I'd suggest something like:
Make a list of renames you want to do.
Convert that into a script that calls gorename <https://godoc.org/golang.org/x/tools/cmd/gorename> a bunch. Test it out, both that the renames apply cleanly and that the results look good.
Post that list somewhere, like an issue or golang-dev, for review, since it is easier to review such a list than the resulting CLs.
Consider asking someone from the core team to apply the renames. (It is a pain to review large automated refactoring changes. This is one of the rare cases in which I treat review requests significantly differently depending on how well I know the author. Known authors can be rubber-stamped, unknown authors require painful manual reviews.)
Do the renames during a quiet period in the tree. The ideal time is basically right now--right before the tree opens for general changes. Then it is likely to apply cleanly and not disrupt too much other work in progress. There's usually a golang-dev email that goes out before the tree opens to coordinate work such as this.
That's probably more than you wanted to know, but hope it is helpful. :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#20304 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeohTlvmIE3q_4YwzBad85-q94xcelopks5tQgm3gaJpZM4NV_Ht>.
|
Hmm, I can’t see it. How are you enclosing it? I think github strips email attachments. |
I just attached it as a PDF to the email.
It started as a Google doc - can I share that with you somehow?
Or is there a better way?
…On Fri, 2 Feb 2018 at 10:53 am, Josh Bleecher Snyder < ***@***.***> wrote:
Hmm, I can’t see it. How are you enclosing it? I think github strips email
attachments.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#20304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AeohTu3J5QxjzwXVLGV3HAq1FZOSyt1rks5tQk6NgaJpZM4NV_Ht>
.
|
Sharing a google doc is fine. Or paste it into a gist (gist.github.com)? Or probably best, just paste all the contents here. |
Issue 20304
Eliminating ssa.Types
You’ve already done all the hard work of mapping these back to types.Type. Eliminating ssa.Types would cause a large volume of renaming Bool to TBOOL, Int8 to TINT8 etc and I don’t really see much value in that, at least in the short term
Moving universe.go to types package
This can be done easily enough but I don’t see that it will have any benefit. It won’t help to eliminate dependencies between types and gc - see the next point.
Removing (cmd/compile/internal/)types and gc dependencies.
There are lots of TODOs in the code about this.
The dependencies are shown in the code from gc/main.go below - some of this is also replicated in ssa/export_test.go.
The definitions end up in types/utils.go.
I’ve looked at each dependency and the analysis follows.
Widthptr = thearch.LinkArch.PtrSize
419 Widthreg = thearch.LinkArch.RegSize
420
421 // initialize types package
422 // (we need to do this to break dependencies that otherwise
423 // would lead to import cycles)
424 types.Widthptr = Widthptr
425 types.Dowidth = dowidth
426 types.Fatalf = Fatalf
427 types.Sconv = func(s *types.Sym, flag, mode int) string {
428 return sconv(s, FmtFlag(flag), fmtMode(mode))
429 }
430 types.Tconv = func(t *types.Type, flag, mode, depth int) string {
431 return tconv(t, FmtFlag(flag), fmtMode(mode), depth)
432 }
433 types.FormatSym = func(sym *types.Sym, s fmt.State, verb rune, mode int) {
434 symFormat(sym, s, verb, fmtMode(mode))
435 }
436 types.FormatType = func(t *types.Type, s fmt.State, verb rune, mode int) {
437 typeFormat(t, s, verb, fmtMode(mode))
438 }
439 types.TypeLinkSym = func(t *types.Type) *obj.LSym {
440 return typenamesym(t).Linksym()
441 }
442 types.FmtLeft = int(FmtLeft)
443 types.FmtUnsigned = int(FmtUnsigned)
444 types.FErr = FErr
445 types.Ctxt = Ctxt
Analysis
The analysis is the result of code reading (and grep) only - no tests have been run (yet).
Dowidth
= func dowidth in gc/align.go.
solution
align.go could be moved to types - more precisely, most of align.go, except func Rnd could be moved. Leave a “small” align.go with just func Rnd in gc?
TypeLinkSym
Uses typenamesym from gc/reflect.go and Linksym in types/sym.go.
Used only to define method *Type.Symbol in types/type.go.
This method is apparently not used.
solution
Remove the *Type.Symbol method and TypeLinkSym.
Widthptr and Ctxt
From “the architecture”
Fatalf
Defined in gc/subr.go
Other
The rest of the constants - FErr, FmtLeft, FmtUnsigned, FmtFlag, fmtMode and functions - sconv, tconv, symFormat, typeFormat all come from gc/fmt.go.
solution
They could be “moved” to the types package (utils.go?) and “copied” to gc/types.go as “convenience constants (and functions)”. Not an ideal solution but workable.
As for Widthptr and Ctxt, could part of gc/main.go be moved back up to cmd/compile/main.go so that “architecture” (and maybe other) values can be propagated to both types and gc? Either that or have a types module up at that level. It seems that the types package needs some initialisation.
Regarding Fatalf, I have no immediate suggestion.
It may very well be that the code above, which works, is better than the alternatives.
Chris Liles
2 February 2018
… On 2 Feb 2018, at 11:17 am, Josh Bleecher Snyder ***@***.***> wrote:
Sharing a google doc is fine. Or paste it into a gist (gist.github.com)? Or probably best, just paste all the contents here.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#20304 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeohTsXx8eF5JB1eS_-k9ViF22X8TX9_ks5tQlQVgaJpZM4NV_Ht>.
|
Some background about internal/types, in case that hasn't been discussed
yet (apologies if I am late to the party, I haven't followed the entire
discussion perhaps):
I've split out the internal types package in a somewhat brute force way
(hence all the TODOs and many ugly initialization steps) in an attempt to
get a better handle on modularizing the front-end (and in turn the ability
to clean it up better). The modularization has worked well for the lexer,
parser, etc. (internal/syntax), but really not so well for internal/types,
at least as it is now.
I'm all for cleaning it up some more, but at the same time I wouldn't
encourage wide-spread changes if there's no clear benefit or a long-term
plan.
In any case, I'd suggest to coordinate with @mdempsky (cc:ed) who is
re-working the import/export format to better cope with repeated, sparsely
used, very large imports.
- gri
On Thu, Feb 1, 2018 at 4:31 PM, Chris Liles <notifications@github.com>
wrote:
… Issue 20304
Eliminating ssa.Types
You’ve already done all the hard work of mapping these back to types.Type.
Eliminating ssa.Types would cause a large volume of renaming Bool to TBOOL,
Int8 to TINT8 etc and I don’t really see much value in that, at least in
the short term
Moving universe.go to types package
This can be done easily enough but I don’t see that it will have any
benefit. It won’t help to eliminate dependencies between types and gc - see
the next point.
Removing (cmd/compile/internal/)types and gc dependencies.
There are lots of TODOs in the code about this.
The dependencies are shown in the code from gc/main.go below - some of
this is also replicated in ssa/export_test.go.
The definitions end up in types/utils.go.
I’ve looked at each dependency and the analysis follows.
Widthptr = thearch.LinkArch.PtrSize
419 Widthreg = thearch.LinkArch.RegSize
420
421 // initialize types package
422 // (we need to do this to break dependencies that otherwise
423 // would lead to import cycles)
424 types.Widthptr = Widthptr
425 types.Dowidth = dowidth
426 types.Fatalf = Fatalf
427 types.Sconv = func(s *types.Sym, flag, mode int) string {
428 return sconv(s, FmtFlag(flag), fmtMode(mode))
429 }
430 types.Tconv = func(t *types.Type, flag, mode, depth int) string {
431 return tconv(t, FmtFlag(flag), fmtMode(mode), depth)
432 }
433 types.FormatSym = func(sym *types.Sym, s fmt.State, verb rune, mode
int) {
434 symFormat(sym, s, verb, fmtMode(mode))
435 }
436 types.FormatType = func(t *types.Type, s fmt.State, verb rune, mode
int) {
437 typeFormat(t, s, verb, fmtMode(mode))
438 }
439 types.TypeLinkSym = func(t *types.Type) *obj.LSym {
440 return typenamesym(t).Linksym()
441 }
442 types.FmtLeft = int(FmtLeft)
443 types.FmtUnsigned = int(FmtUnsigned)
444 types.FErr = FErr
445 types.Ctxt = Ctxt
Analysis
The analysis is the result of code reading (and grep) only - no tests have
been run (yet).
Dowidth
= func dowidth in gc/align.go.
solution
align.go could be moved to types - more precisely, most of align.go,
except func Rnd could be moved. Leave a “small” align.go with just func Rnd
in gc?
TypeLinkSym
Uses typenamesym from gc/reflect.go and Linksym in types/sym.go.
Used only to define method *Type.Symbol in types/type.go.
This method is apparently not used.
solution
Remove the *Type.Symbol method and TypeLinkSym.
Widthptr and Ctxt
From “the architecture”
Fatalf
Defined in gc/subr.go
Other
The rest of the constants - FErr, FmtLeft, FmtUnsigned, FmtFlag, fmtMode
and functions - sconv, tconv, symFormat, typeFormat all come from gc/fmt.go.
solution
They could be “moved” to the types package (utils.go?) and “copied” to
gc/types.go as “convenience constants (and functions)”. Not an ideal
solution but workable.
As for Widthptr and Ctxt, could part of gc/main.go be moved back up to
cmd/compile/main.go so that “architecture” (and maybe other) values can be
propagated to both types and gc? Either that or have a types module up at
that level. It seems that the types package needs some initialisation.
Regarding Fatalf, I have no immediate suggestion.
It may very well be that the code above, which works, is better than the
alternatives.
Chris Liles
2 February 2018
> On 2 Feb 2018, at 11:17 am, Josh Bleecher Snyder <
***@***.***> wrote:
>
> Sharing a google doc is fine. Or paste it into a gist (gist.github.com)?
Or probably best, just paste all the contents here.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub <
#20304 (comment)>, or
mute the thread <https://github.com/notifications/unsubscribe-
auth/AeohTsXx8eF5JB1eS_-k9ViF22X8TX9_ks5tQlQVgaJpZM4NV_Ht>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIIkTy_hp_M1uMavTMOk8LW2DuGDFmp_ks5tQldjgaJpZM4NV_Ht>
.
|
What about just adding more "convenience constants" in the right place? See cmd/compile/internal/gc/types.go.
At a minimum it would help with the package ssa tests, which currently manually initialize some bits of the universe. And it'd be better in the sense that that is where it belongs, and having things where they belong tends to pay dividends.
Or just move Rnd to some other grab-bag util file, like cmd/internal/obj/util.go. It could also really use a doc string. Also, is the cmd/link/internal/ld Rnd identical or not? If identical, it could use the cmd/internal/obj Rnd.
Used in writebarrier.go, I believe.
fmt.go is a train wreck. I would touch/modify it as little as possible. convenience constants seem fine, though.
That seems reasonable to me, but I'd like @mdempsky's opinion. |
And if @griesemer or @mdempsky and I disagree, go with what they say. :) |
I’ve thought about this some more after digesting the replies from Josh/gri.
1. It seems obvious to me now that resolving the dependencies between
(compile/internal)types and gc will require a significant refactoring - my
previous suggestions were only partial workarounds at best. Considering
gri’s comments, it’s probably best to leave this to another time.
2. Looking at the type definitions in ssa, it seems to me that the
definitions currently in ssa/config effectively are the “convenience
constants” that Josh mentioned. Even taking them out of the Config struct
would trigger a large number of changes. So is there anything left to do
there?
3. gc/universe can be moved to the types package if that’s where it
belongs. I doubt that this will have any affect on the types code in
ssa/export_test, but I could be wrong.
I’m very much aware that I’m a babe in the woods in this space so please
tell me if I’m being more of a hinderance than a help here.
…On Fri, 2 Feb 2018 at 11:46 am, Josh Bleecher Snyder < ***@***.***> wrote:
And if @griesemer <https://github.com/griesemer> or @mdempsky
<https://github.com/mdempsky> and I disagree, go with what they say. :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#20304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AeohToiLzpFPaeyWMFLOox3Rjzqlc-SBks5tQlrmgaJpZM4NV_Ht>
.
|
A careful assessment is helpful, even if it doesn’t result in code changes. Sometimes more so. So thank you for that. If you think there are any clear, worthwhile wins, please go ahead and send changes and then we’ll call this fixed enough. And then maybe take your interest and enthusiasm to another issue? :) Those labeled suggested and helpwanted are good places to trawl. |
Change https://golang.org/cl/94256 mentions this issue: |
Disappointingly, I was only able to come up with a minor change which confines the definition of the SSA type pointer struct to a single location - ssa/config.go - rather than two places. This is a very minor code improvement.
|
@ChrisALiles At least for your point 2) I arrived at the same conclusion which is why I didn't move universe to internal/types in the first place. Re: 1), I am not surprised. Making incremental changes in the front-end have become increasingly difficult due to existing interdependencies. I think a long-term plan requires more significant rewrites. |
@ChrisALiles thanks for seeing it through. I hope you continue to dig around elsewhere as well. |
CL 42145 switched package ssa to use types.Type for types. Further cleanup is now possible: Eliminating (in part or in full) ssa.Types, streamlining/eliminating some types.Type methods, moving universe initialization to package types. This is a reminder to do that.
cc @randall77 @mdempsky @griesemer
The text was updated successfully, but these errors were encountered: