Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1291)

Issue 93780044: code review 93780044: go.tools/ssa: create thunks for method expressions T.f. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by adonovan
Modified:
10 years, 4 months ago
Reviewers:
gri
CC:
gri, golang-codereviews, pcc
Visibility:
Public.

Description

go.tools/ssa: create thunks for method expressions T.f. Until now, the same Function was used to represent a method (T)func() and the "method expression" function func(T) formed from it. So the SSA code for this: var buf bytes.Buffer f := Buffer.Bytes f(buf) buf.Bytes() would involve an implicit cast (ChangeType) on line 2. However, compilers based on go/ssa may want to use different calling conventions for them, like gccgo does (see issue 7839). This change decouples them by using an anonymous function called a "thunk", rather like this: f := func(r *bytes.Buffer) []byte { return r.Bytes() } Thunks are similar to method wrappers; both are created by makeWrapper. "Interface method wrappers" were a special case of thunks for direct calls (no indirection/fields) of interface methods. They are now subsumed by thunks and have been deleted. Now that only the needed thunks are built, we don't need to populate the concrete method sets of interface types at all, so (*Program).Method and LookupMethod return nil for them. This results in a slight reduction in function count (>1%) and instruction count (<<1%). Details: go/ssa: - API: ChangeType no longer supports func/method conversions. - API: (*Program).FuncValue now returns nil for abstract (interface) methods. - API: (*Function).RelString simplified. "$bound" is now a suffix not a prefix, and the receiver type is rendered package-relative. - API: Function.Object is now defined for all wrappers too. - API: (*Program).Method and LookupMethod return nil for abstract methods. - emitConv no longer permits (non-identical) Signature->Signature conversions. Added assertion. - add and use isInterface helper - sanity: we check packages after Build, not Create, otherwise cross-package refs might fail. go/pointer: - update tests for new function strings. - pointer_test: don't add non-pointerlike probes to analysis. (The error was checked, but too late, causing a panic.) - fixed a minor bug: if a test probe print(x) was the sole reference to x, no nodes were generated for x. - (reflect.Type).MethodByName: updated due to ssa API changes. Also, fixed incorrect testdata/funcreflect.go expectation for MethodByName on interfaces. oracle: - fix for new FuncValue semantics. - a "pointsto" query on an I.f thunk now returns an error. Fixes issue 7839

Patch Set 1 #

Patch Set 2 : diff -r 8e2bdfe0a5aa https://code.google.com/p/go.tools #

Patch Set 3 : diff -r 8e2bdfe0a5aa https://code.google.com/p/go.tools #

Total comments: 6

Patch Set 4 : diff -r 362a842852db https://code.google.com/p/go.tools #

Unified diffs Side-by-side diffs Delta from patch set Stats (+316 lines, -258 lines) Patch
M go/pointer/gen.go View 1 1 chunk +5 lines, -0 lines 0 comments Download
M go/pointer/pointer_test.go View 1 1 chunk +4 lines, -1 line 0 comments Download
M go/pointer/reflect.go View 1 2 3 2 chunks +13 lines, -11 lines 0 comments Download
M go/pointer/testdata/finalizer.go View 1 1 chunk +1 line, -1 line 0 comments Download
M go/pointer/testdata/func.go View 1 2 chunks +7 lines, -7 lines 0 comments Download
M go/pointer/testdata/funcreflect.go View 1 2 chunks +3 lines, -3 lines 0 comments Download
M go/pointer/testdata/interfaces.go View 1 1 chunk +3 lines, -3 lines 0 comments Download
M go/ssa/builder.go View 1 4 chunks +11 lines, -7 lines 0 comments Download
M go/ssa/create.go View 1 2 chunks +6 lines, -10 lines 0 comments Download
M go/ssa/emit.go View 1 3 chunks +13 lines, -9 lines 0 comments Download
M go/ssa/func.go View 1 1 chunk +23 lines, -24 lines 0 comments Download
M go/ssa/promote.go View 1 2 3 15 chunks +194 lines, -157 lines 0 comments Download
M go/ssa/sanity.go View 1 1 chunk +4 lines, -2 lines 0 comments Download
M go/ssa/source.go View 1 2 1 chunk +8 lines, -9 lines 0 comments Download
M go/ssa/source_test.go View 1 1 chunk +3 lines, -1 line 0 comments Download
M go/ssa/ssa.go View 1 3 chunks +6 lines, -7 lines 0 comments Download
M go/types/selection.go View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M oracle/pointsto.go View 1 2 2 chunks +9 lines, -2 lines 0 comments Download
M oracle/testdata/src/main/pointsto.golden View 1 1 chunk +2 lines, -4 lines 0 comments Download

Messages

Total messages: 4
adonovan
Hello gri@golang.org (cc: golang-codereviews@googlegroups.com, pcc@google.com), I'd like you to review this change to https://code.google.com/p/go.tools
10 years, 6 months ago (2014-04-25 02:48:09 UTC) #1
gri
LGTM https://codereview.appspot.com/93780044/diff/40001/go/ssa/promote.go File go/ssa/promote.go (right): https://codereview.appspot.com/93780044/diff/40001/go/ssa/promote.go#newcode413 go/ssa/promote.go:413: // in call position: C.f(i, 0). Clearly this ...
10 years, 6 months ago (2014-05-01 20:38:58 UTC) #2
adonovan
https://codereview.appspot.com/93780044/diff/40001/go/ssa/promote.go File go/ssa/promote.go (right): https://codereview.appspot.com/93780044/diff/40001/go/ssa/promote.go#newcode413 go/ssa/promote.go:413: // in call position: C.f(i, 0). Clearly this is ...
10 years, 6 months ago (2014-05-02 17:12:06 UTC) #3
adonovan
10 years, 4 months ago (2014-06-11 17:10:29 UTC) #4
*** Submitted as
https://code.google.com/p/go/source/detail?r=d8fabb236a5d&repo=tools ***

go.tools/ssa: create thunks for method expressions T.f.

Until now, the same Function was used to represent a method
(T)func() and the "method expression" function func(T) formed
from it. So the SSA code for this:

    var buf bytes.Buffer
    f := Buffer.Bytes
    f(buf)
    buf.Bytes()

would involve an implicit cast (ChangeType) on line 2.
However, compilers based on go/ssa may want to use different
calling conventions for them, like gccgo does (see issue
7839).  This change decouples them by using an anonymous
function called a "thunk", rather like this:

    f := func(r *bytes.Buffer) []byte { return r.Bytes() }

Thunks are similar to method wrappers; both are created by
makeWrapper.

"Interface method wrappers" were a special case of thunks for
direct calls (no indirection/fields) of interface methods.
They are now subsumed by thunks and have been deleted.  Now
that only the needed thunks are built, we don't need to
populate the concrete method sets of interface types at all,
so (*Program).Method and LookupMethod return nil for them.
This results in a slight reduction in function count (>1%) and
instruction count (<<1%).

Details:

go/ssa:
- API: ChangeType no longer supports func/method conversions.
- API: (*Program).FuncValue now returns nil for abstract
  (interface) methods.
- API: (*Function).RelString simplified.
  "$bound" is now a suffix not a prefix, and the receiver
  type is rendered package-relative.
- API: Function.Object is now defined for all wrappers too.
- API: (*Program).Method and LookupMethod return nil for
  abstract methods.
- emitConv no longer permits (non-identical)
  Signature->Signature conversions.  Added assertion.
- add and use isInterface helper
- sanity: we check packages after Build, not Create, otherwise
  cross-package refs might fail.

go/pointer:
- update tests for new function strings.
- pointer_test: don't add non-pointerlike probes to analysis.
  (The error was checked, but too late, causing a panic.)
- fixed a minor bug: if a test probe print(x) was the sole
  reference to x, no nodes were generated for x.
- (reflect.Type).MethodByName: updated due to ssa API changes.
  Also, fixed incorrect testdata/funcreflect.go expectation
  for MethodByName on interfaces.

oracle:
- fix for new FuncValue semantics.
- a "pointsto" query on an I.f thunk now returns an error.

Fixes issue 7839

LGTM=gri
R=gri
CC=golang-codereviews, pcc
https://codereview.appspot.com/93780044
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b