From 42bd21be1cf54876ce24c489852721049ef293e2 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Mon, 6 Nov 2023 16:28:25 -0500 Subject: [PATCH] cmd/compile: support lookup of functions from export data As of CL 539699, PGO-based devirtualization supports devirtualization of function values in addition to interface method calls. As with CL 497175, we need to explicitly look up functions from export data that may not be imported already. Symbol naming is ambiguous (`foo.Bar.func1` could be a closure or a method), so we simply attempt to do both types of lookup. That said, closures are defined in export data only as OCLOSURE nodes in the enclosing function, which this CL does not yet attempt to expand. For #61577. Change-Id: Ic7205b046218a4dfb8c4162ece3620ed1c3cb40a Reviewed-on: https://go-review.googlesource.com/c/go/+/540258 Reviewed-by: Cherry Mui LUCI-TryBot-Result: Go LUCI Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/noder/unified.go | 45 ++++++++++++++++-- src/cmd/compile/internal/pgo/irgraph.go | 12 ++--- .../internal/test/pgo_devirtualize_test.go | 26 +++++----- .../test/testdata/pgo/devirtualize/devirt.go | 24 ++-------- .../testdata/pgo/devirtualize/devirt.pprof | Bin 1411 -> 1345 bytes .../pgo/devirtualize/mult.pkg/mult.go | 12 +++++ 6 files changed, 73 insertions(+), 46 deletions(-) diff --git a/src/cmd/compile/internal/noder/unified.go b/src/cmd/compile/internal/noder/unified.go index 5948cac58c..a803e53502 100644 --- a/src/cmd/compile/internal/noder/unified.go +++ b/src/cmd/compile/internal/noder/unified.go @@ -30,8 +30,8 @@ var localPkgReader *pkgReader // LookupMethodFunc returns the ir.Func for an arbitrary full symbol name if // that function exists in the set of available export data. // -// This allows lookup of arbitrary methods that aren't otherwise referenced by -// the local package and thus haven't been read yet. +// This allows lookup of arbitrary functions and methods that aren't otherwise +// referenced by the local package and thus haven't been read yet. // // TODO(prattmic): Does not handle instantiation of generic types. Currently // profiles don't contain the original type arguments, so we won't be able to @@ -40,7 +40,7 @@ var localPkgReader *pkgReader // TODO(prattmic): Hit rate of this function is usually fairly low, and errors // are only used when debug logging is enabled. Consider constructing cheaper // errors by default. -func LookupMethodFunc(fullName string) (*ir.Func, error) { +func LookupFunc(fullName string) (*ir.Func, error) { pkgPath, symName, err := ir.ParseLinkFuncName(fullName) if err != nil { return nil, fmt.Errorf("error parsing symbol name %q: %v", fullName, err) @@ -51,6 +51,43 @@ func LookupMethodFunc(fullName string) (*ir.Func, error) { return nil, fmt.Errorf("pkg %s doesn't exist in %v", pkgPath, types.PkgMap()) } + // Symbol naming is ambiguous. We can't necessarily distinguish between + // a method and a closure. e.g., is foo.Bar.func1 a closure defined in + // function Bar, or a method on type Bar? Thus we must simply attempt + // to lookup both. + + fn, err := lookupFunction(pkg, symName) + if err == nil { + return fn, nil + } + + fn, mErr := lookupMethod(pkg, symName) + if mErr == nil { + return fn, nil + } + + return nil, fmt.Errorf("%s is not a function (%v) or method (%v)", fullName, err, mErr) +} + +func lookupFunction(pkg *types.Pkg, symName string) (*ir.Func, error) { + sym := pkg.Lookup(symName) + + // TODO(prattmic): Enclosed functions (e.g., foo.Bar.func1) are not + // present in objReader, only as OCLOSURE nodes in the enclosing + // function. + pri, ok := objReader[sym] + if !ok { + return nil, fmt.Errorf("func sym %v missing objReader", sym) + } + + name := pri.pr.objIdx(pri.idx, nil, nil, false).(*ir.Name) + if name.Op() != ir.ONAME || name.Class != ir.PFUNC { + return nil, fmt.Errorf("func sym %v refers to non-function name: %v", sym, name) + } + return name.Func, nil +} + +func lookupMethod(pkg *types.Pkg, symName string) (*ir.Func, error) { // N.B. readPackage creates a Sym for every object in the package to // initialize objReader and importBodyReader, even if the object isn't // read. @@ -130,7 +167,7 @@ func LookupMethodFunc(fullName string) (*ir.Func, error) { func unified(m posMap, noders []*noder) { inline.InlineCall = unifiedInlineCall typecheck.HaveInlineBody = unifiedHaveInlineBody - pgo.LookupMethodFunc = LookupMethodFunc + pgo.LookupFunc = LookupFunc data := writePkgStub(m, noders) diff --git a/src/cmd/compile/internal/pgo/irgraph.go b/src/cmd/compile/internal/pgo/irgraph.go index be802dabc8..7a7cd20f2b 100644 --- a/src/cmd/compile/internal/pgo/irgraph.go +++ b/src/cmd/compile/internal/pgo/irgraph.go @@ -366,9 +366,9 @@ func addIREdge(callerNode *IRNode, callerName string, call ir.Node, callee *ir.F callerNode.OutEdges[namedEdge] = edge } -// LookupMethodFunc looks up a method in export data. It is expected to be -// overridden by package noder, to break a dependency cycle. -var LookupMethodFunc = func(fullName string) (*ir.Func, error) { +// LookupFunc looks up a function or method in export data. It is expected to +// be overridden by package noder, to break a dependency cycle. +var LookupFunc = func(fullName string) (*ir.Func, error) { base.Fatalf("pgo.LookupMethodFunc not overridden") panic("unreachable") } @@ -425,9 +425,7 @@ func addIndirectEdges(g *IRGraph, namedEdgeMap NamedEdgeMap) { // function may still be available from export data of // a transitive dependency. // - // TODO(prattmic): Currently we only attempt to lookup - // methods because we can only devirtualize interface - // calls, not any function pointer. Generic types are + // TODO(prattmic): Parameterized types/functions are // not supported. // // TODO(prattmic): This eager lookup during graph load @@ -437,7 +435,7 @@ func addIndirectEdges(g *IRGraph, namedEdgeMap NamedEdgeMap) { // devirtualization. Instantiation of generic functions // will likely need to be done at the devirtualization // site, if at all. - fn, err := LookupMethodFunc(key.CalleeName) + fn, err := LookupFunc(key.CalleeName) if err == nil { if base.Debug.PGODebug >= 3 { fmt.Printf("addIndirectEdges: %s found in export data\n", key.CalleeName) diff --git a/src/cmd/compile/internal/test/pgo_devirtualize_test.go b/src/cmd/compile/internal/test/pgo_devirtualize_test.go index 3e264a3f41..c457478a1f 100644 --- a/src/cmd/compile/internal/test/pgo_devirtualize_test.go +++ b/src/cmd/compile/internal/test/pgo_devirtualize_test.go @@ -77,32 +77,30 @@ go 1.19 }, // ExerciseFuncConcrete { - pos: "./devirt.go:178:18", + pos: "./devirt.go:173:36", callee: "AddFn", }, - // TODO(prattmic): Export data lookup for function value callees not implemented. - //{ - // pos: "./devirt.go:179:15", - // callee: "mult.MultFn", - //}, + { + pos: "./devirt.go:173:15", + callee: "mult.MultFn", + }, // ExerciseFuncField { - pos: "./devirt.go:218:13", + pos: "./devirt.go:207:35", callee: "AddFn", }, - // TODO(prattmic): Export data lookup for function value callees not implemented. - //{ - // pos: "./devirt.go:219:19", - // callee: "mult.MultFn", - //}, + { + pos: "./devirt.go:207:19", + callee: "mult.MultFn", + }, // ExerciseFuncClosure // TODO(prattmic): Closure callees not implemented. //{ - // pos: "./devirt.go:266:9", + // pos: "./devirt.go:249:27", // callee: "AddClosure.func1", //}, //{ - // pos: "./devirt.go:267:15", + // pos: "./devirt.go:249:15", // callee: "mult.MultClosure.func1", //}, } diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go index 63de3d3c3f..ac238f6dea 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go +++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go @@ -170,13 +170,7 @@ func ExerciseFuncConcrete(iter int, a1, a2 AddFunc, m1, m2 mult.MultFunc) int { // If they were not mutually exclusive (for example, two // AddFunc calls), then we could not definitively select the // correct callee. - // - // TODO(prattmic): Export data lookup for function value - // callees not implemented, meaning the type is unavailable. - //sink += int(m(42, int64(a(1, 2)))) - - v := selectA(i)(one(i), 2) - val += int(m(42, int64(v))) + val += int(m(42, int64(selectA(i)(one(i), 2)))) } return val } @@ -210,13 +204,7 @@ func ExerciseFuncField(iter int, a1, a2 AddFunc, m1, m2 mult.MultFunc) int { // If they were not mutually exclusive (for example, two // AddFunc calls), then we could not definitively select the // correct callee. - // - // TODO(prattmic): Export data lookup for function value - // callees not implemented, meaning the type is unavailable. - //sink += int(ops.m(42, int64(ops.a(1, 2)))) - - v := ops.a(1, 2) - val += int(ops.m(42, int64(v))) + val += int(ops.m(42, int64(ops.a(1, 2)))) } return val } @@ -258,13 +246,7 @@ func ExerciseFuncClosure(iter int, a1, a2 AddFunc, m1, m2 mult.MultFunc) int { // If they were not mutually exclusive (for example, two // AddFunc calls), then we could not definitively select the // correct callee. - // - // TODO(prattmic): Export data lookup for function value - // callees not implemented, meaning the type is unavailable. - //sink += int(m(42, int64(a(1, 2)))) - - v := a(1, 2) - val += int(m(42, int64(v))) + val += int(m(42, int64(a(1, 2)))) } return val } diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.pprof index de064582ff0b22b455a0e7e893fb8bda16bba3e3..2a27f1bb50d21b8126c319453c69e845d444d049 100644 GIT binary patch literal 1345 zcmV-H1-|+piwFP!00004|GZR9Y!qb}c4l{`-_Eu(Z)dmd+jfCr_;IYVLr0oIq6dmL zdXSiKQZ%}q9hOaJXEwVt1WioFgBlMSqw%6(frP6+ zQAEu5%~Ypomd1mJ&QAAxp7(v8=Y7Au=j_45@BV)N^DhRZEa8BcvLpl9``{_nH8LO8rSJIm`7r2{u25e@_tG|^bDD}=}6d)Ofl4m;$b3%fMYSf?u_gHIh~ zhZ!ib!we84TFxlyK(csxW;>BWobMyd=1qDwEXmIUaH=_;Ox={>NT(R-i| z`!v~Dr7G|ML~k6}n~?-HK^O3H>`I1!p=IdDeoZvmstf`|=PwUPLV+m*QpCOc5|?{8 z;MAV^6thLz8%Q5sSfK82KpzDtq#x%};!<Frak@f=@Pm(8x&Z5$VE{zba>jT%%fX?n zpaU7kdlK(RhJ@b|< z3H=nWE2MN4OYqVRGtdja#s+#}5C=8UxJV5w#Xmk}coim?0TqUDNXr=; zRr#;tr6o(*E-k~8uQG5Uaa(|49M)uGTnD1zZ|O#=0uMp-&Nm-sBw?8r^OxgHERJ15 zRR$TNpU+K5!t$hnjz7i#rarx4dV*MiCwDOkMcBmj7NLYCO*SS}86=24J8eqBilpC4 zJeAU$x`it#U}F`?y^80z9ozS+ZEK~ovqB+d+i#f7nr~K}9racaR^0mIPI+UsO8=~FX4r0- zu5VXdv*!CX*D<&I4aaOWTa_R%+b!Fy`6m6a8&%Wx8_l}wm~}63T3*F91E(FZN3{}E z{?R3AS+4okh|@{U-8-F@U2i-0ZmZaiHQEt7;du7;Mx`}<7xNQSrvC>Uk5IGJOD}z? zd$qDLGI2|}6?&VkI~UzQ;hIzPHfY5{@@-tvt%RPv-CEx%2D1^m!79sXPS?tto!Wh& z8?gi~xm^-jepV$!?8!ptw% zWa!zOe9vwvwDcv*UKMOcsu1(zD8)JftZ!ph@orp0|WkH>$b7^JLv|tDT4R zZ-!&q+D_6JwAtkP?Xcxo;~g2--CmY#Lfp`aczsF{08E#Zr^2vXEUigGSS= z`D>=ax?3IHFgiYV``EfM%bZ%fb~O17OMW&3ryW@0)*aQh|L~sy00960XL61c{R#j8 DqcwbN literal 1411 zcmV-}1$_D+iwFP!00004|GZRPjMP*c{yH=5p6Tqg?{sEo&d&aI`+c9OYnbkq1@=NO z0A2M$G-_gkG1%>NXD8dA$+RVCV$!G=pkzS}CM0emZlWl%3fcI-EFdI=fW`|XkYI>j zs5eT2CPq|D&vcRj$*_9i#dLbk^Stl#yzhBW@0ppOd+p@$6DJF70#OiS6DSUe2Z95; z=lVC2_s4_%Une9UQFw7_DJF$138*0!Q9xr_WaL4KM=`v04*_Ff5HJQExI<**QHe)! zeEdBE#-W#haX=V}NjWD0O5n2#`(qM~5^#h9YAA&gz+zTR$pn$z?9`1+W zn{&U!Seof*gd@B-ORRQ8Ry%;lyvWHt0tYDwe*AEhrBOsq;@QK5jNmq+g#a*!j65W8 zz(R2JtisaC$QX++wipvQh(U1vSdyjLC|e4D@hZtiS}3G}g0S;!BGs_giTCX#coICK zmV^w>h>Yw?fYNx^TT5XVSvVMWkwq*BSP&WcDTzlMKJx(yO+kr-rXY*6Vp3iqaFB=K ztIJa?-I?Y{2YLK_L#GogLhghd&WVg{3IKa32u`2BO0zT{(E-o2&;CR})Krr)JFH1wXmH_4P<)+#R3R({;-<1dn1^n^Wo#9yk?CK*U3xF{eyX6=9 zBtRH%zqBKSGcZ>qa0ZIFD0a(F7bQU5_=htD7U25=fd%NnJz}@~TV4WG#P2>!l4s#q zc$rct3%$5k?3Ul?l>qhNLo)=>z2j5{>y03qLjGBmlYPmoH2U!Ttp4!OLVni6k@3-g+93&uE z*p_AK!I02$N5W+h5CuUH#BgwNDJDx}R56EQ5FGw}_R7@(90Y&NUb#9T|5yEw+Tm(e z%`{w<)*avW)VN;r)s(H-j%(Ut2j#8*j%l9zGhaRG)imB@y%TTOZUGqG}t?Np~QNlmnDl58U)vBhURBg|w+nT9(hU<|> zS@X2Ns6>*bilYu0TiUp}Yl~6WtFCeHq^29{NL%hY!`7!Ptv-DxsizS#{V$ZUDJ(Xu zB@@0>?Mg`=T6b%y?%NyGJC>~<#VQlcGQy16Xt$Bk)O=f?QdhNWgJSun_kY!>O;<|y z`KCu6)tdQ)YAxH!KRMA*wp5$0G+ru>I<{Pw8;r`eM%xeQL;eIAP^n%1kTq`KWMG=Z zHSXK`ddJr5hG(?5ef^AG6WQL-+H!f^R@byOeIrAqN#E8-)Y7s8$E$`}Za*V8Dx(pW zt^2lDwTzNJW$4onS1qINs$&cA93o_YOc98S+&a#`nIi2 zn1%|?U00tn%D!o+Tyv+U)u%UEYPz|$bj>u?+yAyF&9ydZR(bVk$yJA!-Ocr;E0UpI*;!J(YQMbJ=ws R`!fIl|Nl!0vc|m%001!ur0W0x diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult.pkg/mult.go b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult.pkg/mult.go index 64f405ff9e..113a5e1a7e 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult.pkg/mult.go +++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult.pkg/mult.go @@ -35,10 +35,16 @@ func (NegMult) Multiply(a, b int) int { type MultFunc func(int64, int64) int64 func MultFn(a, b int64) int64 { + for i := 0; i < 1000; i++ { + sink++ + } return a * b } func NegMultFn(a, b int64) int64 { + for i := 0; i < 1000; i++ { + sink++ + } return -1 * a * b } @@ -47,6 +53,9 @@ func MultClosure() MultFunc { // Explicit closure to differentiate from AddClosure. c := 1 return func(a, b int64) int64 { + for i := 0; i < 1000; i++ { + sink++ + } return a * b * int64(c) } } @@ -55,6 +64,9 @@ func MultClosure() MultFunc { func NegMultClosure() MultFunc { c := 1 return func(a, b int64) int64 { + for i := 0; i < 1000; i++ { + sink++ + } return -1 * a * b * int64(c) } } -- 2.44.0