Support MULX returning ValueTuple #61336

pentp · 2021-11-09T01:07:03Z

Based on #37928, fixes #11782.

dotnet-issue-labeler · 2021-11-09T01:07:08Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-11-09T01:07:10Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on #37928, fixes #11782.

Author:	pentp
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `new-api-needs-documentation`
Milestone:	-

pentp · 2021-11-10T16:28:41Z

So I tried to get the stale PR #37928 working again, but there are two issues that I'm having trouble with:

The TYP_STRUCT returned by the intrinsic is always marked with should not be enregistered because: field of a dependently promoted struct and thus is always stored to memory before it can be used.
As an edge case, compiling MultiplyNoFlags2 itself (through reflection) fails because in this case it does NOT store the tuple in memory and tries to return the value in multiple registers (which isn't possible on win-x64).

JitDump for Math:BigMul(long,long,byref):long

pentp · 2021-11-10T16:47:04Z

Math.BigMul diff is a slight improvement (due to the memory stores/loads), but actually it should generate this:

mulx     rax, rdx, rcx
mov      qword ptr [r8], rdx
ret

pentp · 2021-11-10T17:35:06Z

@EgorBo maybe you have some good hints?

SingleAccretion · 2021-11-10T17:47:27Z

The TYP_STRUCT returned by the intrinsic is always marked with should not be enregistered because: field of a dependently promoted struct and thus is always stored to memory before it can be used.

Speaking as a front-end person...

There is an invariant in the Jit that promoted struct locals must be either:

Eliminated from the IR completely (replaced with fields). This is known as "independent" promotion.
Marked as do-not-enregister, so that their constituent field locals do not participate in the SSA-based optimizations, and in the backend, writes to fields affect the parent local.
Treated specially (look for CanBeReplacedWithItsField special handling as one example of this).

As for the immediate cause of spilling, it is because this must be added as a new "special" case, along with the multireg calls, here:

runtime/src/coreclr/jit/morphblock.cpp

Lines 695 to 708 in b07b26d

    
           #if FEATURE_MULTIREG_RET 
        
               // If this is a multi-reg return, we will not do any morphing of this node. 
        
               if (m_src->IsMultiRegCall()) 
        
               { 
        
                   assert(m_dst->OperGet() == GT_LCL_VAR); 
        
                   JITDUMP("Not morphing a multireg call return\n"); 
        
                   m_transformationDecision = BlockTransformation::SkipCallSrc; 
        
                   m_result                 = m_asg; 
        
               } 
        
               else if (m_dst->IsMultiRegLclVar() && !m_src->IsMultiRegNode()) 
        
               { 
        
                   m_dst->AsLclVar()->ClearMultiReg(); 
        
               } 
        
           #endif // FEATURE_MULTIREG_RET

am11 · 2021-11-10T19:11:40Z

src/libraries/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.cs

@@ -3707,6 +3707,7 @@ public abstract partial class Bmi2 : System.Runtime.Intrinsics.X86.X86Base
        public static new bool IsSupported { get { throw null; } }
        public static uint MultiplyNoFlags(uint left, uint right) { throw null; }
        public unsafe static uint MultiplyNoFlags(uint left, uint right, uint* low) { throw null; }
+        internal unsafe static (uint Lower, uint Upper) MultiplyNoFlags2(uint left, uint right) { throw null; }


nit: changes in /ref are not required for internal APIs.

am11 · 2021-11-10T19:12:55Z

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/Bmi2.cs

+        ///   MULX r32a, r32b, reg/m32
+        /// The above native signature does not directly correspond to the managed signature.
+        /// </summary>
+        internal static unsafe (uint Lower, uint Upper) MultiplyNoFlags2(uint left, uint right) => MultiplyNoFlags2(left, right);


I know this is an existing pattern for intrinsic, but would be less confusing if it is something like:
=> throw new NotImplementedException("Intrinsic must be implemented by the runtime.");
instead of recursive call.

I note that the recursiveness is an implementation detail that allows (easy) detection of when the intrinsic must be expanded:

runtime/src/coreclr/jit/importer.cpp

Lines 3740 to 3741 in 1d352fc

// The recursive non-virtual calls to Jit intrinsics are must-expand by convention.

mustExpand = mustExpand || (gtIsRecursiveCall(method) && !(methodFlags & CORINFO_FLG_VIRTUAL));

Ah, I see. Thanks for the link. IMO, it would have made the managed APIs more readable if the detection was based off of some descriptive construct.

This implementation is actually there to support calls using reflection. It's mostly a theoretical concern, but precludes a need for special handling of reflection for HW intrinsics.

However, for internal methods throwing might be a valid option, not sure.

ghost · 2021-12-10T23:01:04Z

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

Support MULX returning ValueTuple

0acf35d

dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Nov 9, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 9, 2021

Try some fixes

05eda03

am11 reviewed Nov 10, 2021

View reviewed changes

ghost closed this Dec 10, 2021

ghost locked as resolved and limited conversation to collaborators Jan 10, 2022

pentp deleted the mulx-v2 branch July 1, 2023 18:28

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MULX returning ValueTuple #61336

Support MULX returning ValueTuple #61336

pentp commented Nov 9, 2021

dotnet-issue-labeler bot commented Nov 9, 2021

ghost commented Nov 9, 2021

pentp commented Nov 10, 2021

pentp commented Nov 10, 2021 •

edited

Loading

pentp commented Nov 10, 2021

SingleAccretion commented Nov 10, 2021 •

edited

Loading

am11 Nov 10, 2021

am11 Nov 10, 2021

SingleAccretion Nov 10, 2021

am11 Nov 10, 2021

pentp Nov 10, 2021

pentp Nov 10, 2021

ghost commented Dec 10, 2021

	// The recursive non-virtual calls to Jit intrinsics are must-expand by convention.
	mustExpand = mustExpand \|\| (gtIsRecursiveCall(method) && !(methodFlags & CORINFO_FLG_VIRTUAL));

Support MULX returning ValueTuple #61336

Support MULX returning ValueTuple #61336

Conversation

pentp commented Nov 9, 2021

dotnet-issue-labeler bot commented Nov 9, 2021

ghost commented Nov 9, 2021

pentp commented Nov 10, 2021

pentp commented Nov 10, 2021 • edited Loading

pentp commented Nov 10, 2021

SingleAccretion commented Nov 10, 2021 • edited Loading

am11 Nov 10, 2021

Choose a reason for hiding this comment

am11 Nov 10, 2021

Choose a reason for hiding this comment

SingleAccretion Nov 10, 2021

Choose a reason for hiding this comment

am11 Nov 10, 2021

Choose a reason for hiding this comment

pentp Nov 10, 2021

Choose a reason for hiding this comment

pentp Nov 10, 2021

Choose a reason for hiding this comment

ghost commented Dec 10, 2021

pentp commented Nov 10, 2021 •

edited

Loading

SingleAccretion commented Nov 10, 2021 •

edited

Loading