cmd/compile: performance regression with returned structs in 1.27.0 due to CL 748200

[jleahy]

First of all, thank you for all your hard work on go 1.27.0.

On upgrading from 1.26.4 to 1.27.0 I noticed a major performance regression on one of my benchmarks. This is a simple reproducer:

type T struct {
	s    []byte
	x, y int
}

func newT(s []byte) T { return T{s: s} }

func Append(s []byte) []byte {
	t := newT(s)
	t.s = append(t.s, 1)
	return t.s
}

In 1.27.0 the function is 47% slower (linux/amd64). What's happening is that when you have a value-returning constructor whose result is assigned to a local and then mutated the struct gets built twice on the stack instead of once. Bisecting reveals that CL 748200 is responsible. Amusingly this was supposed to fix #77720, a very similar problem. The fix in 1.27.0 does fix that case, but it also causes broader performance regressions.

I believe the fix is as simple as the following:

--- a/src/cmd/compile/internal/ssa/_gen/generic.rules                                                                                             
+++ b/src/cmd/compile/internal/ssa/_gen/generic.rules                                                                                             
@@ -823,7 +823,7 @@                                                                                                                               
 // Load from a region just copied by Move can read directly from the source.                                                                     
 (Load <t1> op:(OffPtr [o1] p1) move:(Move [n] p2 src mem))                                                                                       
    && o1 >= 0 && o1+t1.Size() <= n && isSamePtr(p1, p2)                                                                                          
-   && !isVolatile(src)                                                                                                                           
+   && !isVolatile(src) && !isStackPtr(src)                                                                                                       
    => @move.Block (Load <t1> (OffPtr <op.Type> [o1] src) mem)                                                                                    

This patch results in identical codegen for my example as 1.26.4 whilst still ensuring that the original issue remains fixed. I'd be happy to raise a PR if that would be useful.

[Dorbmon]

I investigated this and can reproduce the regression. The proposed !isStackPtr(src) guard fixes the reproducer, but it is broader than necessary: it disables Load-through-Move forwarding for every stack-backed source. That regresses cases where the source must remain materialized but the destination copy can still be eliminated.

The problematic shape is specifically:

Load(dstField, Move(dst, src, VarDef(dst, mem)))

CL 748200 forwards the load to src, but retains VarDef(dst, mem) as its memory dependency. Consequently, the source temporary still appears to be read, load/store forwarding cannot see its initialization stores through the VarDef, and dead-auto cannot eliminate it.

Eliminating the temporary before this rewrite does not solve the problem: at that point the temporary is still the source of the whole-struct Move. It only becomes unread after the field loads have been forwarded and folded into its initialization stores.

A narrower fix is to add a specialized rule before the existing one:

(Load <t1> op1:(OffPtr [o1] p1)
    move:(Move [n] p2 src vardef:(VarDef mem)))
    && o1 >= 0 && o1+t1.Size() <= n
    && ssa.IsSamePtr(p1, p2)
    && !ssa.IsVolatile(src)
    && isStackPtr(src)
    && isVarDefFor(vardef, p2)
    => @move.Block (Load <t1> (OffPtr <op1.Type> [o1] src) mem)

This bypasses only the VarDef belonging to the Move destination. Restricting it to stack sources ensures the moved load cannot fault. Existing load/store forwarding can then eliminate the source loads, after which dead-auto removes ~r0.

On the reproducer, this reduces the function from 235 to 155 bytes and the stack frame from 152 to 112 bytes, with ~r0 eliminated. I also added coverage ensuring the original #77720 optimization and useful forwarding from materialized stack sources remain intact. The amd64 codegen checks and the compiler SSA test suites pass.

I have the patch and regression tests ready and can send a CL if this approach looks reasonable.

[randall77]

It seems that forwarding the load to the move source really only helps if we can remove all uses of the move destination. If uses of the move destination still exist, the rehoming of the load isn't really all that helpful, as the move still has to happen. Maybe it would help a bit with load latency, but it doesn't get rid of instructions, just moves them. In the particular case of this issue, is it that rehoming partially keeps both the source and destination alive, where previously the source was later deadcoded somehow?

@JunyangShao Do your SROA CLs help with this?

[JunyangShao]

@randall77 Yes SROA helped. I think after CL 820249, the example code compiles to 0 struct builds, even better than 1.26.4.
I will add this as an example to that CL