Issue 92780044: go.tools: expose expression addressability from the typ... (Closed)
Descriptiongo.tools: expose expression addressability from the type checker
This is used by the ssa package to form loads from addressable
expressions wherever possible instead of loading entire aggregates
and extracting elements (which can lead to inefficient code).
Patch Set 1 #Patch Set 2 : diff -r 8e2bdfe0a5aa https://code.google.com/p/go.tools #
Total comments: 8
Patch Set 3 : diff -r 9abbb23a14f6 https://code.google.com/p/go.tools #
MessagesTotal messages: 13
For review only/not for submission until we unfreeze.
Sign in to reply to this message.
gri will no doubt have opinions about the proposed types API. Obviously this change must wait until the tree thaws. https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go File go/loader/pkginfo.go (right): https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go#newco... go/loader/pkginfo.go:40: func (info *PackageInfo) TypeAndAddyOf(e ast.Expr) (types.Type, bool) { I don't like abbreviating by removing from the middle of a word, except when very well established (src, pkg, msg). I don't think this function is justified, since clients can already call info.Types[e] if they need to access fields other than the type. Also, I'd like to make it a precondition of TypeOf (and by extension, this function) that e is a true expression, not a defining ast.Ident. Then we can remove the hack below, which isn't really needed since clients can always know whether an Ident is defining or not from the syntax in which they encounter it, and I would rather make them call ObjectOf in that case (which they are usually doing subsequently anyway). https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go#newco... go/loader/pkginfo.go:90: func (info *PackageInfo) IsType(e ast.Expr) bool { This function could be eliminated if we expose the typexpr mode too. https://codereview.appspot.com/92780044/diff/20001/go/ssa/builder.go File go/ssa/builder.go (right): https://codereview.appspot.com/92780044/diff/20001/go/ssa/builder.go#newcode459 go/ssa/builder.go:459: if fn.Pkg.isAddr(e) { This call to isAddr and the call to ValueOf just above cause repeated hashtable lookups. I would prefer to get rid of the wrappers and get the TypeAndValue directly, once. But more importantly, I don't think this is the right approach. We don't want to replace every (r-mode) reference to an addressable object with addr() and load, most of which will then get thrown away by the ssa lifting pass. Can't you just test for addressability in the few places it matters, e.g. when indexing structs and arrays? https://codereview.appspot.com/92780044/diff/20001/go/types/api.go File go/types/api.go (right): https://codereview.appspot.com/92780044/diff/20001/go/types/api.go#newcode124 go/types/api.go:124: type TypeAndValue struct { It might be worth converging 'TypeAndValue' with 'operand', although this is a larger change, obviously. In particular I think we might want to expose more of the operand.mode case discrimination, specifically: - typexpr: this is needed in several places in ssa.builder, e.g. to distinguish a conversion int(x) from a call f(x). There is some code in PackageInfo that tries to recompute it syntactically, but it's much less efficient. - mapindex: a map index can be both an lvalue and an rvalue even though it is not strictly addressable. This could be useful for optimizations in ssa. The other cases are less compelling (though I could contrive examples). https://codereview.appspot.com/92780044/diff/20001/go/types/check.go File go/types/check.go (right): https://codereview.appspot.com/92780044/diff/20001/go/types/check.go#newcode245 go/types/check.go:245: func (check *checker) recordTypeAndValue(x ast.Expr, typ Type, isaddr bool, val exact.Value) { It would be better to pass the operand mode here. Its values are more informative than "true" and "false", and I think we may want the additional information anyway.
Sign in to reply to this message.
https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go File go/loader/pkginfo.go (right): https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go#newco... go/loader/pkginfo.go:40: func (info *PackageInfo) TypeAndAddyOf(e ast.Expr) (types.Type, bool) { On 2014/04/25 15:09:01, adonovan wrote: > I don't like abbreviating by removing from the middle of a word, except when > very well established (src, pkg, msg). > > I don't think this function is justified, since clients can already call > info.Types[e] if they need to access fields other than the type. > > Also, I'd like to make it a precondition of TypeOf (and by extension, this > function) that e is a true expression, not a defining ast.Ident. Then we can > remove the hack below, which isn't really needed since clients can always know > whether an Ident is defining or not from the syntax in which they encounter it, > and I would rather make them call ObjectOf in that case (which they are usually > doing subsequently anyway). agreed https://codereview.appspot.com/92780044/diff/20001/go/types/api.go File go/types/api.go (right): https://codereview.appspot.com/92780044/diff/20001/go/types/api.go#newcode124 go/types/api.go:124: type TypeAndValue struct { On 2014/04/25 15:09:01, adonovan wrote: > It might be worth converging 'TypeAndValue' with 'operand', although this is a > larger change, obviously. In particular I think we might want to expose more of > the operand.mode case discrimination, specifically: > > - typexpr: this is needed in several places in ssa.builder, e.g. to distinguish > a conversion int(x) from a call f(x). There is some code in PackageInfo that > tries to recompute it syntactically, but it's much less efficient. > > - mapindex: a map index can be both an lvalue and an rvalue even though it is > not strictly addressable. This could be useful for optimizations in ssa. > > The other cases are less compelling (though I could contrive examples). Fair enough. Maybe we can expose the mode. Or perhaps even Operand. If so, I would like to make the change. https://codereview.appspot.com/92780044/diff/20001/go/types/check.go File go/types/check.go (right): https://codereview.appspot.com/92780044/diff/20001/go/types/check.go#newcode245 go/types/check.go:245: func (check *checker) recordTypeAndValue(x ast.Expr, typ Type, isaddr bool, val exact.Value) { On 2014/04/25 15:09:01, adonovan wrote: > It would be better to pass the operand mode here. Its values are more > informative than "true" and "false", and I think we may want the additional > information anyway. we might want to expose the operand
Sign in to reply to this message.
Let's expose the mode, but not the whole operand, since it's redundant to store the Expr in both the map key and value, and there are a lot of these things. On 30 April 2014 15:53, <gri@golang.org> wrote: > > https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go > File go/loader/pkginfo.go (right): > > https://codereview.appspot.com/92780044/diff/20001/go/ > loader/pkginfo.go#newcode40 > go/loader/pkginfo.go:40: func (info *PackageInfo) TypeAndAddyOf(e > ast.Expr) (types.Type, bool) { > On 2014/04/25 15:09:01, adonovan wrote: > >> I don't like abbreviating by removing from the middle of a word, >> > except when > >> very well established (src, pkg, msg). >> > > I don't think this function is justified, since clients can already >> > call > >> info.Types[e] if they need to access fields other than the type. >> > > Also, I'd like to make it a precondition of TypeOf (and by extension, >> > this > >> function) that e is a true expression, not a defining ast.Ident. Then >> > we can > >> remove the hack below, which isn't really needed since clients can >> > always know > >> whether an Ident is defining or not from the syntax in which they >> > encounter it, > >> and I would rather make them call ObjectOf in that case (which they >> > are usually > >> doing subsequently anyway). >> > > agreed > > > https://codereview.appspot.com/92780044/diff/20001/go/types/api.go > File go/types/api.go (right): > > https://codereview.appspot.com/92780044/diff/20001/go/ > types/api.go#newcode124 > go/types/api.go:124: type TypeAndValue struct { > On 2014/04/25 15:09:01, adonovan wrote: > >> It might be worth converging 'TypeAndValue' with 'operand', although >> > this is a > >> larger change, obviously. In particular I think we might want to >> > expose more of > >> the operand.mode case discrimination, specifically: >> > > - typexpr: this is needed in several places in ssa.builder, e.g. to >> > distinguish > >> a conversion int(x) from a call f(x). There is some code in >> > PackageInfo that > >> tries to recompute it syntactically, but it's much less efficient. >> > > - mapindex: a map index can be both an lvalue and an rvalue even >> > though it is > >> not strictly addressable. This could be useful for optimizations in >> > ssa. > > The other cases are less compelling (though I could contrive >> > examples). > > Fair enough. Maybe we can expose the mode. Or perhaps even Operand. If > so, I would like to make the change. > > > https://codereview.appspot.com/92780044/diff/20001/go/types/check.go > File go/types/check.go (right): > > https://codereview.appspot.com/92780044/diff/20001/go/ > types/check.go#newcode245 > go/types/check.go:245: func (check *checker) recordTypeAndValue(x > ast.Expr, typ Type, isaddr bool, val exact.Value) { > On 2014/04/25 15:09:01, adonovan wrote: > >> It would be better to pass the operand mode here. Its values are more >> informative than "true" and "false", and I think we may want the >> > additional > >> information anyway. >> > > we might want to expose the operand > > https://codereview.appspot.com/92780044/ >
Sign in to reply to this message.
I like to make this change since I want to fully understand the consequences. Perhaps tomorrow of Fri. We cannot submit of course, until in about a month. - gri On Wed, Apr 30, 2014 at 2:52 PM, Alan Donovan <adonovan@google.com> wrote: > Let's expose the mode, but not the whole operand, since it's redundant to > store the Expr in both the map key and value, and there are a lot of these > things. > > > > On 30 April 2014 15:53, <gri@golang.org> wrote: > >> >> https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go >> File go/loader/pkginfo.go (right): >> >> https://codereview.appspot.com/92780044/diff/20001/go/ >> loader/pkginfo.go#newcode40 >> go/loader/pkginfo.go:40: func (info *PackageInfo) TypeAndAddyOf(e >> ast.Expr) (types.Type, bool) { >> On 2014/04/25 15:09:01, adonovan wrote: >> >>> I don't like abbreviating by removing from the middle of a word, >>> >> except when >> >>> very well established (src, pkg, msg). >>> >> >> I don't think this function is justified, since clients can already >>> >> call >> >>> info.Types[e] if they need to access fields other than the type. >>> >> >> Also, I'd like to make it a precondition of TypeOf (and by extension, >>> >> this >> >>> function) that e is a true expression, not a defining ast.Ident. Then >>> >> we can >> >>> remove the hack below, which isn't really needed since clients can >>> >> always know >> >>> whether an Ident is defining or not from the syntax in which they >>> >> encounter it, >> >>> and I would rather make them call ObjectOf in that case (which they >>> >> are usually >> >>> doing subsequently anyway). >>> >> >> agreed >> >> >> https://codereview.appspot.com/92780044/diff/20001/go/types/api.go >> File go/types/api.go (right): >> >> https://codereview.appspot.com/92780044/diff/20001/go/ >> types/api.go#newcode124 >> go/types/api.go:124: type TypeAndValue struct { >> On 2014/04/25 15:09:01, adonovan wrote: >> >>> It might be worth converging 'TypeAndValue' with 'operand', although >>> >> this is a >> >>> larger change, obviously. In particular I think we might want to >>> >> expose more of >> >>> the operand.mode case discrimination, specifically: >>> >> >> - typexpr: this is needed in several places in ssa.builder, e.g. to >>> >> distinguish >> >>> a conversion int(x) from a call f(x). There is some code in >>> >> PackageInfo that >> >>> tries to recompute it syntactically, but it's much less efficient. >>> >> >> - mapindex: a map index can be both an lvalue and an rvalue even >>> >> though it is >> >>> not strictly addressable. This could be useful for optimizations in >>> >> ssa. >> >> The other cases are less compelling (though I could contrive >>> >> examples). >> >> Fair enough. Maybe we can expose the mode. Or perhaps even Operand. If >> so, I would like to make the change. >> >> >> https://codereview.appspot.com/92780044/diff/20001/go/types/check.go >> File go/types/check.go (right): >> >> https://codereview.appspot.com/92780044/diff/20001/go/ >> types/check.go#newcode245 >> go/types/check.go:245: func (check *checker) recordTypeAndValue(x >> ast.Expr, typ Type, isaddr bool, val exact.Value) { >> On 2014/04/25 15:09:01, adonovan wrote: >> >>> It would be better to pass the operand mode here. Its values are more >>> informative than "true" and "false", and I think we may want the >>> >> additional >> >>> information anyway. >>> >> >> we might want to expose the operand >> >> https://codereview.appspot.com/92780044/ >> > >
Sign in to reply to this message.
Thanks Robert. On Wed, Apr 30, 2014 at 2:59 PM, Robert Griesemer <gri@golang.org> wrote: > I like to make this change since I want to fully understand the > consequences. Perhaps tomorrow of Fri. We cannot submit of course, until in > about a month. > - gri > > > On Wed, Apr 30, 2014 at 2:52 PM, Alan Donovan <adonovan@google.com> wrote: > >> Let's expose the mode, but not the whole operand, since it's redundant to >> store the Expr in both the map key and value, and there are a lot of these >> things. >> >> >> >> On 30 April 2014 15:53, <gri@golang.org> wrote: >> >>> >>> https://codereview.appspot.com/92780044/diff/20001/go/loader/pkginfo.go >>> File go/loader/pkginfo.go (right): >>> >>> https://codereview.appspot.com/92780044/diff/20001/go/ >>> loader/pkginfo.go#newcode40 >>> go/loader/pkginfo.go:40: func (info *PackageInfo) TypeAndAddyOf(e >>> ast.Expr) (types.Type, bool) { >>> On 2014/04/25 15:09:01, adonovan wrote: >>> >>>> I don't like abbreviating by removing from the middle of a word, >>>> >>> except when >>> >>>> very well established (src, pkg, msg). >>>> >>> >>> I don't think this function is justified, since clients can already >>>> >>> call >>> >>>> info.Types[e] if they need to access fields other than the type. >>>> >>> >>> Also, I'd like to make it a precondition of TypeOf (and by extension, >>>> >>> this >>> >>>> function) that e is a true expression, not a defining ast.Ident. Then >>>> >>> we can >>> >>>> remove the hack below, which isn't really needed since clients can >>>> >>> always know >>> >>>> whether an Ident is defining or not from the syntax in which they >>>> >>> encounter it, >>> >>>> and I would rather make them call ObjectOf in that case (which they >>>> >>> are usually >>> >>>> doing subsequently anyway). >>>> >>> >>> agreed >>> >>> >>> https://codereview.appspot.com/92780044/diff/20001/go/types/api.go >>> File go/types/api.go (right): >>> >>> https://codereview.appspot.com/92780044/diff/20001/go/ >>> types/api.go#newcode124 >>> go/types/api.go:124: type TypeAndValue struct { >>> On 2014/04/25 15:09:01, adonovan wrote: >>> >>>> It might be worth converging 'TypeAndValue' with 'operand', although >>>> >>> this is a >>> >>>> larger change, obviously. In particular I think we might want to >>>> >>> expose more of >>> >>>> the operand.mode case discrimination, specifically: >>>> >>> >>> - typexpr: this is needed in several places in ssa.builder, e.g. to >>>> >>> distinguish >>> >>>> a conversion int(x) from a call f(x). There is some code in >>>> >>> PackageInfo that >>> >>>> tries to recompute it syntactically, but it's much less efficient. >>>> >>> >>> - mapindex: a map index can be both an lvalue and an rvalue even >>>> >>> though it is >>> >>>> not strictly addressable. This could be useful for optimizations in >>>> >>> ssa. >>> >>> The other cases are less compelling (though I could contrive >>>> >>> examples). >>> >>> Fair enough. Maybe we can expose the mode. Or perhaps even Operand. If >>> so, I would like to make the change. >>> >>> >>> https://codereview.appspot.com/92780044/diff/20001/go/types/check.go >>> File go/types/check.go (right): >>> >>> https://codereview.appspot.com/92780044/diff/20001/go/ >>> types/check.go#newcode245 >>> go/types/check.go:245: func (check *checker) recordTypeAndValue(x >>> ast.Expr, typ Type, isaddr bool, val exact.Value) { >>> On 2014/04/25 15:09:01, adonovan wrote: >>> >>>> It would be better to pass the operand mode here. Its values are more >>>> informative than "true" and "false", and I think we may want the >>>> >>> additional >>> >>>> information anyway. >>>> >>> >>> we might want to expose the operand >>> >>> https://codereview.appspot.com/92780044/ >>> >> >> >
Sign in to reply to this message.
Hopefully https://codereview.appspot.com/110880043 will make this one unnecessary. Please comment on 110880043 and if you're happy with it, feel free to delete this one. Thanks. - gri
Sign in to reply to this message.
On 2014/07/10 18:16:22, gri wrote: > Hopefully https://codereview.appspot.com/110880043 will make this one > unnecessary. > > Please comment on 110880043 and if you're happy with it, feel free to delete > this one. Thanks. > > - gri Thanks Robert. I much prefer the approach you have taken in that CL. I have no particular comments to make there. Once that CL lands I intend to update this CL to cover only the go/ssa changes (per Alan's code review above), unless Alan is planning to make those changes himself. Peter
Sign in to reply to this message.
I think Alan already has a CL pending with the respective ssa changes. - gri On Thu, Jul 10, 2014 at 12:22 PM, <pcc@google.com> wrote: > On 2014/07/10 18:16:22, gri wrote: > >> Hopefully https://codereview.appspot.com/110880043 will make this one >> unnecessary. >> > > Please comment on 110880043 and if you're happy with it, feel free to >> > delete > >> this one. Thanks. >> > > - gri >> > > Thanks Robert. I much prefer the approach you have taken in that CL. I > have no particular comments to make there. > > Once that CL lands I intend to update this CL to cover only the go/ssa > changes (per Alan's code review above), unless Alan is planning to make > those changes himself. > > Peter > > https://codereview.appspot.com/92780044/ >
Sign in to reply to this message.
This one? https://codereview.appspot.com/107650043/ Unless I am misreading it, it looks like it does not implement the optimization that this CL implements. Peter On Thu, Jul 10, 2014 at 12:33 PM, Robert Griesemer <gri@golang.org> wrote: > I think Alan already has a CL pending with the respective ssa changes. > - gri > > > On Thu, Jul 10, 2014 at 12:22 PM, <pcc@google.com> wrote: > >> On 2014/07/10 18:16:22, gri wrote: >> >>> Hopefully https://codereview.appspot.com/110880043 will make this one >>> unnecessary. >>> >> >> Please comment on 110880043 and if you're happy with it, feel free to >>> >> delete >> >>> this one. Thanks. >>> >> >> - gri >>> >> >> Thanks Robert. I much prefer the approach you have taken in that CL. I >> have no particular comments to make there. >> >> Once that CL lands I intend to update this CL to cover only the go/ssa >> changes (per Alan's code review above), unless Alan is planning to make >> those changes himself. >> >> Peter >> >> https://codereview.appspot.com/92780044/ >> > >
Sign in to reply to this message.
Ok. Feel free to take a stab at it. Alan is in London this and next week, so he will see it tomorrow morning. - gri On Thu, Jul 10, 2014 at 12:48 PM, Peter Collingbourne <pcc@google.com> wrote: > This one? > https://codereview.appspot.com/107650043/ > > Unless I am misreading it, it looks like it does not implement the > optimization that this CL implements. > > Peter > > > On Thu, Jul 10, 2014 at 12:33 PM, Robert Griesemer <gri@golang.org> wrote: > >> I think Alan already has a CL pending with the respective ssa changes. >> - gri >> >> >> On Thu, Jul 10, 2014 at 12:22 PM, <pcc@google.com> wrote: >> >>> On 2014/07/10 18:16:22, gri wrote: >>> >>>> Hopefully https://codereview.appspot.com/110880043 will make this one >>>> unnecessary. >>>> >>> >>> Please comment on 110880043 and if you're happy with it, feel free to >>>> >>> delete >>> >>>> this one. Thanks. >>>> >>> >>> - gri >>>> >>> >>> Thanks Robert. I much prefer the approach you have taken in that CL. I >>> have no particular comments to make there. >>> >>> Once that CL lands I intend to update this CL to cover only the go/ssa >>> changes (per Alan's code review above), unless Alan is planning to make >>> those changes himself. >>> >>> Peter >>> >>> https://codereview.appspot.com/92780044/ >>> >> >> >
Sign in to reply to this message.
Hi Peter, you're right that my CL does not add the optimization. Let's do that as a follow-up CL since the current one is just refactoring; it's pretty much a one-liner. On 10 July 2014 15:48, Peter Collingbourne <pcc@google.com> wrote: > This one? > https://codereview.appspot.com/107650043/ > > Unless I am misreading it, it looks like it does not implement the > optimization that this CL implements. > > Peter > > > On Thu, Jul 10, 2014 at 12:33 PM, Robert Griesemer <gri@golang.org> wrote: > >> I think Alan already has a CL pending with the respective ssa changes. >> - gri >> >> >> On Thu, Jul 10, 2014 at 12:22 PM, <pcc@google.com> wrote: >> >>> On 2014/07/10 18:16:22, gri wrote: >>> >>>> Hopefully https://codereview.appspot.com/110880043 will make this one >>>> unnecessary. >>>> >>> >>> Please comment on 110880043 and if you're happy with it, feel free to >>>> >>> delete >>> >>>> this one. Thanks. >>>> >>> >>> - gri >>>> >>> >>> Thanks Robert. I much prefer the approach you have taken in that CL. I >>> have no particular comments to make there. >>> >>> Once that CL lands I intend to update this CL to cover only the go/ssa >>> changes (per Alan's code review above), unless Alan is planning to make >>> those changes himself. >>> >>> Peter >>> >>> https://codereview.appspot.com/92780044/ >>> >> >> >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
