JIT: update impFixupStructReturnType to handle GT_RET_EXPR

Project: dotnet/runtime | Language: C# | Concepts: compiler-internals, refactoring, testing-strategy | Difficulty: senior
Link: https://github.com/dotnet/runtime/pull/132986

The problem

The JIT compiler's impFixupStructReturnType function did not correctly handle cases where the value being returned was a GT_RET_EXPR node (a placeholder for an inline candidate's return value). This led to incorrect code generation when struct-returning calls were forwarded through intrinsics like Unsafe.BitCast, resulting in test failures or runtime errors for certain struct-returning methods.

Why it's non-trivial

A naive fix might only check for direct call nodes (GT_CALL), missing the fact that GT_RET_EXPR can also represent a call that returns a struct via a return buffer. Handling only direct calls would fail for inlined or forwarded return values, leading to subtle bugs in optimized or inlined code paths.

The approach

The core change is in impFixupStructReturnType, which now recognizes both direct calls and GT_RET_EXPR placeholders that reference calls needing a return buffer. The function materializes the return value into a temp local variable in both cases.

1. Recognizing both GT_CALL and GT_RET_EXPR:

-    if (op->IsCall() && op->AsCall()->ShouldHaveRetBufArg())
+    GenTreeCall* retBufCall = nullptr;
+
+    if (op->IsCall())
+    {
+        retBufCall = op->AsCall();
+    }
+    else if (op->OperIs(GT_RET_EXPR))
+    {
+        // The placeholder was created during this importation, so it cannot have been
+        // substituted yet; only "gtSubstExpr" can form chains of GT_RET_EXPRs, and those
+        // are walked later by "UpdateInlineReturnExpressionPlaceHolder".
+        assert(op->AsRetExpr()->gtSubstExpr == nullptr);
+        retBufCall = op->AsRetExpr()->gtInlineCandidate;
+    }
+
+    if ((retBufCall != nullptr) && retBufCall->ShouldHaveRetBufArg())

What this does:

• Checks if the node is a call or a GT_RET_EXPR (inline return value placeholder).
• If it's a GT_RET_EXPR, extracts the underlying call candidate.
• Proceeds only if the call requires a return buffer.

2. Materializing the return buffer into a temp:

-        // This must be one of those 'special' helpers that don't really have a return buffer, but instead
-        // use it as a way to keep the trees cleaner with fewer address-taken temps. Well now we have to
-        // materialize the return buffer as an address-taken temp. Then we can return the temp.
-        //
         unsigned tmpNum = lvaGrabTemp(true DEBUGARG("pseudo return buffer"));
 
         // No need to spill anything as we're about to return.
         impStoreToTemp(tmpNum, op, CHECK_SPILL_NONE);
 
         op = gtNewLclvNode(tmpNum, info.compRetType);
-        JITDUMP("\nimpFixupStructReturnType: created a pseudo-return buffer for a special helper\n");
+        JITDUMP("\nimpFixupStructReturnType: created a pseudo-return buffer\n");
         DISPTREE(op);
 
         return op;

What this does:

• Allocates a temporary variable to hold the struct return value.
• Stores the result into the temp, then returns the temp variable.
• The comment was updated to remove references to obsolete "special helpers."

3. Test coverage:
A new regression test is added to ensure struct returns via Unsafe.BitCast and similar patterns are handled correctly:

+    [Fact]
+    public static void TestEntryPoint()
+    {
+        V2<int> result = Unbox(GetV2());
+
+        Assert.Equal(1, result.X);
+        Assert.Equal(2, result.Y);
+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    private static V2<int> GetV2() =>
+        Unsafe.BitCast<Vector64<int>, V2<int>>(CreateVector());
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    private static Vector64<int> CreateVector() => Vector64.Create(1, 2);
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static V2<int> Unbox(object value) => (V2<int>)value;

What this does:

• Exercises the code path where a struct is returned via an intrinsic and then unboxed, covering the previously broken scenario.

From the review thread

jakobbotsch: I would just remove this comment -- we don't have these 'special' helpers anymore, and also I don't really understand what the second part is saying (impStoreStruct does that for both GT_RET_EXPR and GT_CALL).

AndyAyersMS: Comment was trying to say that we aren't wrapping the call, we're wrapping the ret expr.

Removed.

Takeaways

• When handling IR transformations, always account for placeholder or forwarding nodes (like GT_RET_EXPR) in addition to direct operation nodes.
• Remove or update obsolete comments to prevent confusion about current code behavior and supported scenarios.
• Regression tests should be added for subtle IR or ABI-handling bugs, especially when fixing issues exposed by inlining or intrinsic forwarding.