Refactor and cleanup work for passing and returning struct types #6198

briansull · 2016-07-08T23:10:05Z

Replaces argOrReturnTypeForStruct with two new methods:
getArgTypeForStruct - Provides information on how to pass a struct type
getReturnTypeForStruct - Provides information on how to return a struct type

A struct can be passed or returned in the following different ways:
1. A struct type may be passed/returned as a primitive type when its size is small
2. A struct type may be passed/returned by reference to a copy
3. A struct type may be passed/returned by value using multiple registers
4. A struct type may be passed by value using a copy on the stack

briansull · 2016-07-08T23:18:31Z

@dotnet-bot Test Windows_NT arm64 Checked

briansull · 2016-07-08T23:30:42Z

@dotnet-bot Test Windows_NT arm64 Checked

briansull · 2016-07-09T01:13:46Z

@dotnet-bot Test Windows_NT arm64 Checked

briansull · 2016-07-09T02:12:38Z

@dotnet/jit-contrib PTAL

briansull · 2016-07-09T02:13:19Z

@dotnet-bot test Windows_NT arm64 Checked pri1r2r

BruceForstall · 2016-07-09T04:28:37Z

On ARM, a struct can also be passed partially in registers and partially on the stack.

BruceForstall · 2016-07-09T04:33:08Z

src/jit/compiler.h

-    var_types    argOrReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd, bool forReturn);
+    enum structPassingKind { SPK_Unknown,             // Invalid value, never returned
+                             SPK_PrimitiveType,       // The struct is passed/returned using a primitive type
+                             SPK_ByValue,             // The struct is passed/returned by value, usually in multiple registers


Does this include both on stack, in registers, and split between registers and stack? (if so, probably the comment should say that)

Yes I will add to the comment. This method is classifing what should be done, it doesn't actually implement the passing or returning of structure.

BruceForstall · 2016-07-09T05:08:04Z

LGTM module comments. I like how all this ABI info is encapsulated in these functions. Can they be used in more cases, to remove similar logic elsewhere?

RussKeldorph · 2016-07-11T17:01:37Z

src/jit/compiler.cpp

+//
+// Notes:
+//    About returning TYP_STRUCT:
+//        Whenever the return type is TYP_STRUCT it always means multiple registers


For consistency with surrounding comments, you should change "return type" to "return value" even though in some sense what you have now is valid.

In my comments when referring to the 'var_types' return value, I used "this method's return value" and in the method getReturnTypeForStruct the phrase "return type" refers to how the struct will be returned.
Also in out coding conventions we have defined that method header comment contains a
section called "Return Type:" which describes what the function returns/

I'm failing to resolve your distinction with immediately surrounding comments. For example:

Just above: "When *wbPassStruct is SPK_ByValue or SPK_ByValueAsHfa the _return value:_..."

Just below: "If there are two or more elements in the HFA type then the _return value_ is TYP_STRUCT...

But perhaps it's just me. If you are convinced you wrote what you meant to write, then disregard.

Also, coding convention specifies "Return Value:" not "Return Type:", though I'm not sure how that's relevant.

"this method's return value" means the value returned by getArgTypeForStruct()
The only occurances of "return type" remainig in my comments is this phrase:
"and the target supports multireg return types then the return value is TYP_STRUCT"

The comments that you refered to now reads:

// Return Value:
// For wbPassStruct you can pass a 'nullptr' and nothing will be written
// or returned for that out parameter.
// When *wbPassStruct is SPK_PrimitiveType this method's return value
// is the primitive type used to pass the struct.
// When *wbPassStruct is SPK_ByReference this method's return value
// is TYP_UNKNOWN and the struct type is passed by reference to a copy
// When *wbPassStruct is SPK_ByValue or SPK_ByValueAsHfa this method's return value
// can be TYP_STRUCT and the struct type is passed using multiple registers.
// or can be TYP_UNKNOWN and the struct type is passed by value (for x86 and ARM32)

RussKeldorph · 2016-07-11T17:02:41Z

Regarding the description of this change: "4. A struct type may be passed/returned by value using a copy on the stack," I believe you should remove the "/returned" part.

[Edit] This is done.

CarolEidt · 2016-07-11T17:14:42Z

src/jit/compiler.cpp

-//                false if we are asking for this in a parameter passing context
+//    clsHnd     - the handle for the struct type, used when may have 
+//                 an HFA or if we need the GC layout for an object ref.
+//    structSize - the size of the struct type, cannot be zero


Is there a reason you swapped the order of the argument descriptions, relative to how they are passed?

I will switch these to the correct order.

briansull · 2016-07-11T20:21:37Z

@dotnet-bot Test Windows_NT arm64 Checked

CarolEidt · 2016-07-11T20:34:18Z

src/jit/compiler.cpp

+//    When *wbReturnStruct is SPK_ByReference this method's return value
+//       is TYP_UNKNOWN when the struct type is returned using a return buffer
+//    When *wbReturnStruct is SPK_ByValue or SPK_ByValueAsHfa this method's return value
+//       is TYP_STRUCT when the struct type is returned using multiple registers.


These comments don't appear to cover all the cases - if these cases can't occur, the comment make it clear:
I think it's the case that when *wbReturnStruct is SPK_ByReference, the return value is always TYP_UNKNOWN, AND (not "when") the struct type is (always) returned using a return buffer.
I also think it's the case that when *wbReturnStruct is SPK_ByValue or SPK_ByValueAsHfa, then the methods return value is always TYP_STRUCT, AND (not "when") the struct type is returned using multiple registers.

Yes, I have undated this comment as you suggested.
I also checked the other method getArgTypeForStruct in this area and found it to be correct.

CarolEidt · 2016-07-11T20:35:58Z

src/jit/compiler.cpp

+//    When *wbPassStruct is SPK_ByReference this method's return value
+//       is TYP_UNKNOWN and the struct type is passed by reference to a copy
+//    When *wbPassStruct is SPK_ByValue or SPK_ByValueAsHfa this method's return value 
+//       can be TYP_STRUCT and the struct type is passed using multiple registers.


Should this say "is passed using multiple registers, if sufficient argument registers are available, and is otherwise passed by value on the stack"?

In the Notes section I cover this:

// About returning TYP_STRUCT:
// Whenever this method's return value is TYP_STRUCT it usually means that multiple
// registers will be used to pass this struct.
// The only exception occurs if all of the parameters registers are used up
// then we must use stack slots instead. In such a case the amount of stack needed
// is always equal to the structSize (rounded up to the next pointer size)
//

briansull · 2016-07-11T21:43:41Z

@dotnet-bot Test Ubuntu x64 Checked Build and Test

briansull · 2016-07-11T23:42:18Z

@dotnet-bot test Windows_NT arm64 Checked pri1r2r

briansull · 2016-07-11T23:52:30Z

@dotnet-bot Test Windows_NT Arm64 Checked Pri1r2r

briansull · 2016-07-11T23:52:52Z

@dotnet-bot Test Windows_NT arm64 Checked

briansull · 2016-07-11T23:53:41Z

@dotnet-bot Test Windows_NT Arm64 Checked Pri1r2r

CarolEidt · 2016-07-12T00:06:32Z

LGTM

Replaces argOrReturnTypeForStruct with two new methods: getArgTypeForStruct - Provides information on how to pass a struct type getReturnTypeForStruct - Provides information on how to return a struct type A struct can be passed or return in the following different ways: 1. A struct type may be passed/returned as a primitive type when its size is small 2. A struct type may be passed/returned by reference to a copy 3. A struct type may be passed/returned by value using multiple registers 4. A struct type may be passed by value using a copy on the stack Incorporated code review feedback with expanded comments.

briansull · 2016-07-12T01:51:11Z

@dotnet-bot Test Windows_NT Arm64 Checked Pri1r2r

briansull · 2016-07-12T02:10:59Z

@dotnet-bot Test Windows_NT arm64 Checked

briansull · 2016-07-12T02:35:48Z

@dotnet-bot Test Windows_NT arm64 Checked

briansull · 2016-07-12T02:37:33Z

@dotnet-bot Test Linux ARM Emulator Cross Debug Build

briansull · 2016-07-12T02:38:13Z

@dotnet-bot Test Windows_NT Arm64 Checked Pri1r2r

briansull · 2016-07-12T02:39:02Z

@dotnet-bot Test Linux ARM Emulator Cross Release Build

briansull · 2016-07-12T08:20:05Z

@dotnet-bot Test Linux ARM Emulator Cross Release Build

Refactor and cleanup work for passing and returning struct types Commit migrated from dotnet/coreclr@b33bd67

dnfclas added the cla-already-signed label Jul 8, 2016

briansull force-pushed the multireg-refactor branch from 1703fd4 to 1ee30a8 Compare July 8, 2016 23:29

briansull mentioned this pull request Jul 9, 2016

[ARM-softfp/Linux] Get precise type for struct #6182

Merged

briansull force-pushed the multireg-refactor branch from 1ee30a8 to 3a0b626 Compare July 9, 2016 01:12

BruceForstall reviewed Jul 9, 2016
View reviewed changes

RussKeldorph reviewed Jul 11, 2016
View reviewed changes

CarolEidt reviewed Jul 11, 2016
View reviewed changes

briansull force-pushed the multireg-refactor branch from 3a0b626 to cae20f9 Compare July 11, 2016 20:09

CarolEidt reviewed Jul 11, 2016
View reviewed changes

briansull force-pushed the multireg-refactor branch from cae20f9 to 8ee7fe7 Compare July 11, 2016 23:29

briansull force-pushed the multireg-refactor branch from 8ee7fe7 to 6530924 Compare July 12, 2016 01:39

briansull merged commit b33bd67 into dotnet:master Jul 12, 2016

briansull deleted the multireg-refactor branch July 12, 2016 17:06

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#6198 from briansull/multireg-refactor

ffe60cd

Refactor and cleanup work for passing and returning struct types Commit migrated from dotnet/coreclr@b33bd67

Refactor and cleanup work for passing and returning struct types #6198

Refactor and cleanup work for passing and returning struct types #6198

Uh oh!

Conversation

briansull commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briansull commented Jul 8, 2016

Uh oh!

briansull commented Jul 8, 2016

Uh oh!

briansull commented Jul 9, 2016

Uh oh!

briansull commented Jul 9, 2016

Uh oh!

briansull commented Jul 9, 2016

Uh oh!

BruceForstall commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull Jul 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussKeldorph Jul 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussKeldorph commented Jul 11, 2016 • edited by briansull Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull commented Jul 11, 2016

Uh oh!

briansull commented Jul 11, 2016

Uh oh!

briansull commented Jul 11, 2016

Uh oh!

briansull commented Jul 11, 2016

Uh oh!

briansull commented Jul 11, 2016

Uh oh!

CarolEidt commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 12, 2016

Uh oh!

briansull commented Jul 8, 2016 •

edited

Loading

briansull Jul 11, 2016 •

edited

Loading

RussKeldorph Jul 11, 2016 •

edited

Loading

RussKeldorph commented Jul 11, 2016 •

edited by briansull

Loading