From 363713223385476b87dc5f26d267df8c67d13006 Mon Sep 17 00:00:00 2001 From: thepudds Date: Sat, 3 Jun 2023 00:56:31 +0000 Subject: [PATCH] cmd/compile/internal/devirtualize: devirtualize methods in other packages if current package has a concrete reference The new PGO-driven indirect call specialization from CL 492436 in theory should allow for devirtualization on methods in another package when those methods are directly referenced in the current package. However, inline.InlineImpossible was checking for a zero-length fn.Body and would cause devirtualization to fail with a debug log message like: "should not PGO devirtualize (*Speaker1).Speak: no function body" Previously, the logic in inline.InlineImpossible was only called on local functions, but with PGO-based devirtualization, it can now be called on imported functions, where inlinable imported functions will have a zero-length fn.Body but a non-nil fn.Inl. We update inline.InlineImpossible to handle imported functions by adding a call to typecheck.HaveInlineBody in the check that was previously failing. For the test, we need to have a hopefully temporary workaround of adding explicit references to the callees in another package for devirtualization to work. CL 497175 or similar should enable removing this workaround. Fixes #60561 Updates #59959 Change-Id: I48449b7d8b329d84151bd3b506b8093c262eb2a3 GitHub-Last-Rev: 2d53c55fd895ad8fefd25510a6e6969e89d54a6d GitHub-Pull-Request: golang/go#60565 Reviewed-on: https://go-review.googlesource.com/c/go/+/500155 Run-TryBot: thepudds Reviewed-by: Cherry Mui Reviewed-by: Michael Pratt TryBot-Result: Gopher Robot --- src/cmd/compile/internal/devirtualize/pgo.go | 4 +- src/cmd/compile/internal/inline/inl.go | 5 ++- .../internal/test/pgo_devirtualize_test.go | 12 +++--- .../test/testdata/pgo/devirtualize/devirt.go | 38 +++++++----------- .../testdata/pgo/devirtualize/devirt.pprof | Bin 682 -> 699 bytes .../testdata/pgo/devirtualize/devirt_test.go | 6 ++- .../testdata/pgo/devirtualize/mult/mult.go | 32 +++++++++++++++ 7 files changed, 62 insertions(+), 35 deletions(-) create mode 100644 src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult/mult.go diff --git a/src/cmd/compile/internal/devirtualize/pgo.go b/src/cmd/compile/internal/devirtualize/pgo.go index 69c421ca5a..979483a46f 100644 --- a/src/cmd/compile/internal/devirtualize/pgo.go +++ b/src/cmd/compile/internal/devirtualize/pgo.go @@ -357,11 +357,11 @@ func rewriteCondCall(call *ir.CallExpr, curfn, callee *ir.Func, concretetyp *typ elseBlock.Append(call) } else { // Copy slice so edits in one location don't affect another. - thenRet := append([]ir.Node(nil), retvars...) + thenRet := append([]ir.Node(nil), retvars...) thenAsList := ir.NewAssignListStmt(pos, ir.OAS2, thenRet, []ir.Node{concreteCall}) thenBlock.Append(typecheck.Stmt(thenAsList)) - elseRet := append([]ir.Node(nil), retvars...) + elseRet := append([]ir.Node(nil), retvars...) elseAsList := ir.NewAssignListStmt(pos, ir.OAS2, elseRet, []ir.Node{call}) elseBlock.Append(typecheck.Stmt(elseAsList)) } diff --git a/src/cmd/compile/internal/inline/inl.go b/src/cmd/compile/internal/inline/inl.go index c61d6d2234..4ae7fa95d2 100644 --- a/src/cmd/compile/internal/inline/inl.go +++ b/src/cmd/compile/internal/inline/inl.go @@ -414,8 +414,9 @@ func InlineImpossible(fn *ir.Func) string { return reason } - // If fn has no body (is defined outside of Go), cannot inline it. - if len(fn.Body) == 0 { + // If a local function has no fn.Body (is defined outside of Go), cannot inline it. + // Imported functions don't have fn.Body but might have inline body in fn.Inl. + if len(fn.Body) == 0 && !typecheck.HaveInlineBody(fn) { reason = "no function body" return reason } diff --git a/src/cmd/compile/internal/test/pgo_devirtualize_test.go b/src/cmd/compile/internal/test/pgo_devirtualize_test.go index 5ddd626962..d524ddb3a2 100644 --- a/src/cmd/compile/internal/test/pgo_devirtualize_test.go +++ b/src/cmd/compile/internal/test/pgo_devirtualize_test.go @@ -57,11 +57,11 @@ go 1.19 want := []devirtualization{ { - pos: "./devirt.go:81:21", - callee: "Mult.Multiply", + pos: "./devirt.go:61:21", + callee: "mult.Mult.Multiply", }, { - pos: "./devirt.go:81:31", + pos: "./devirt.go:61:31", callee: "Add.Add", }, } @@ -115,8 +115,10 @@ func TestPGODevirtualize(t *testing.T) { // Copy the module to a scratch location so we can add a go.mod. dir := t.TempDir() - - for _, file := range []string{"devirt.go", "devirt_test.go", "devirt.pprof"} { + if err := os.Mkdir(filepath.Join(dir, "mult"), 0755); err != nil { + t.Fatalf("error creating dir: %v", err) + } + for _, file := range []string{"devirt.go", "devirt_test.go", "devirt.pprof", filepath.Join("mult", "mult.go")} { if err := copyFile(filepath.Join(dir, file), filepath.Join(srcDir, file)); err != nil { t.Fatalf("error copying %s: %v", file, err) } 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 3f22093b34..390b6c350a 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go +++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt.go @@ -11,32 +11,12 @@ package devirt -type Multiplier interface { - Multiply(a, b int) int -} - -type Adder interface { - Add(a, b int) int -} +import "example.com/pgo/devirtualize/mult" var sink int -type Mult struct{} - -func (Mult) Multiply(a, b int) int { - for i := 0; i < 1000; i++ { - sink++ - } - return a * b -} - -type NegMult struct{} - -func (NegMult) Multiply(a, b int) int { - for i := 0; i < 1000; i++ { - sink++ - } - return -1 * a * b +type Adder interface { + Add(a, b int) int } type Add struct{} @@ -60,7 +40,7 @@ func (Sub) Add(a, b int) int { // Exercise calls mostly a1 and m1. // //go:noinline -func Exercise(iter int, a1, a2 Adder, m1, m2 Multiplier) { +func Exercise(iter int, a1, a2 Adder, m1, m2 mult.Multiplier) { for i := 0; i < iter; i++ { a := a1 m := m1 @@ -81,3 +61,13 @@ func Exercise(iter int, a1, a2 Adder, m1, m2 Multiplier) { sink += m.Multiply(42, a.Add(1, 2)) } } + +func init() { + // TODO: until https://golang.org/cl/497175 or similar lands, + // we need to create an explicit reference to callees + // in another package for devirtualization to work. + m := mult.Mult{} + m.Multiply(42, 0) + n := mult.NegMult{} + n.Multiply(42, 0) +} 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 b72f7cf4b3a7117a8187ed01f81938e8d90c0cbf..5fe5dd606fcd06884036ad130e4cd4e4e1c65c2c 100644 GIT binary patch literal 699 zcmV;s0z~~EiwFP!00004|Fn`%XysHC#?AlRy!Pevwc_<5G!0c^)!f%xD|XR^MZt|l zaA&ESmxT91^N*LLw65+(L=jYQD-@wtD1rzs#8T`+TnIvUg((a(%xq>ei&=JPhTOiw z%q-rly2#~_@B8lgoy&Ru;D?((AO3sz`wO~^1Q2x@Ng#jtV0ZsVVAiZgg086AWgOvuzUHVJ zJkR4A7}zkI_H?TW8nC~9o7ELGG>_pgzUO8H7B?dR45n(&wwfRT`*OdjD`PcS8;}5u zF%h%fBhj9GTWC)s5!l~1PA{q%%-GrKMKxo$+HE8OW9+=Ds;Uw&+hylfT?_UUeU56m z8)s3FQ^iZmq@a?Qm9&w#Nty>RluBr}b1d`D9&$Nzbw8S6_7o zWwJ(JA!R-wQR=x7>81R2h9=UzLad`x!@rl&$So7EPsykeNnA#S^TE{-X3-9HuVu=* z5kG2B`fR4Pp!X!!+^CH6QQ75t<&YxINH2Y(TZWOpxV+R^q;z>~UCL>?5Z=(72Xp&F}J1;6(g2#L%OE7^a^t!plH#Jy;;{5Y- zf+}&mAWDXYWc+mKCBrwBmkg75QkTt^p&6@b1GZssb}r zxdIK`&}H+xW&`R_9Nbt?B{YI-_}y?K2$m`sfrd3*HW!->P@ve^{#KQ0qhKoD9Ku{V zzGeeBCIkUgy(7t+4d9^|+^MNjy|ilq;-DxB9v6GNya_*Yt*;Od#on9!!QnJ6 zHs9_K4yVm2`UO=|D~NrTQpt|;Fr$JU=d>PLVU#jE3Z0bBu#F1o+EGAaHzE#O_mV8P zeD5^tE;~;7QAYAKA%0|AK5?VS^%?0$0V6@2SXoBW#3pV;%AXxL#Eyd4^BM8Nj3uGv zlZ>TVwK!I0edI#Yz7?~s8`1CC#+Wd_ZLq}lQbuRTVk<1P`+=35{I%+Yp7{LP|0^3T z8<;9LUg&ns_R0_4Bo9~V&!22RswKS-4U{*2IXWHQ=v#Sc_i20V0RG7RtXjO-KSeuZ z*;VEq$H%{iwY=9|b{yIvSr8L9>hyBYcNUlCe_C2tpp^9H=H|(<7nHFyqw+uNP8$9D QJ^%p!|MKzW!(jvf0DsC!`Tzg` diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go index 03c966f6de..f4cbbb8069 100644 --- a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go +++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/devirt_test.go @@ -13,14 +13,16 @@ package devirt import ( "testing" + + "example.com/pgo/devirtualize/mult" ) func BenchmarkDevirt(b *testing.B) { var ( a1 Add a2 Sub - m1 Mult - m2 NegMult + m1 mult.Mult + m2 mult.NegMult ) Exercise(b.N, a1, a2, m1, m2) diff --git a/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult/mult.go b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult/mult.go new file mode 100644 index 0000000000..8a026a52f5 --- /dev/null +++ b/src/cmd/compile/internal/test/testdata/pgo/devirtualize/mult/mult.go @@ -0,0 +1,32 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// WARNING: Please avoid updating this file. +// See the warning in ../devirt.go for more details. + +package mult + +var sink int + +type Multiplier interface { + Multiply(a, b int) int +} + +type Mult struct{} + +func (Mult) Multiply(a, b int) int { + for i := 0; i < 1000; i++ { + sink++ + } + return a * b +} + +type NegMult struct{} + +func (NegMult) Multiply(a, b int) int { + for i := 0; i < 1000; i++ { + sink++ + } + return -1 * a * b +} -- 2.50.0