Skip to content

Refactor HasMultiRegRetVal and impFixupCallStructReturn. #36465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sandreenko merged 9 commits into from
May 15, 2020
Merged

Refactor HasMultiRegRetVal and impFixupCallStructReturn. #36465

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
93e5224
Fix target definitions.
May 6, 2020
9d1acfd
Fix failures after a recent HW changes.
May 7, 2020
bb8cab8
Add a const getter for `ReturnTypeDesc` from a call.
May 14, 2020
2a7647f
Refactor `HasMultiRegRetVal` and `impFixupCallStructReturn`.
May 14, 2020
7856087
Delete an extra `.` in some dumps.
May 14, 2020
81dda67
Add an additional check that `ReturnTypeDesc` is initialized.
May 14, 2020
25b005b
Remove old `const_cast` around `GetReturnTypeDesc`.
May 14, 2020
82ee494
Replace non-const `GetReturnTypeDesc` with other methods.
May 15, 2020
1bdda32
Fix uninitialized `gtSpillFlags, gtOtherRegs, gtReturnTypeDesc` in `f…
May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refactor HasMultiRegRetVal and impFixupCallStructReturn. 
Delete an unnecessary nested condition and make checks more straightforward.
  • Loading branch information
Sergey Andreenko committed May 14, 2020
commit 2a7647f1027a7977e0d5a508d1924cd45cabdf52
27 changes: 19 additions & 8 deletions src/coreclr/src/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4234,16 +4234,27 @@ struct GenTreeCall final : public GenTree
//
bool HasMultiRegRetVal() const
{
#if defined(TARGET_X86)
return varTypeIsLong(gtType);
#elif FEATURE_MULTIREG_RET && defined(TARGET_ARM)
return varTypeIsLong(gtType) || (varTypeIsStruct(gtType) && !HasRetBufArg());
#ifdef FEATURE_MULTIREG_RET
#if defined(TARGET_X86) || defined(TARGET_ARM)
if (varTypeIsLong(gtType))
{
return true;
}
#elif defined(FEATURE_HFA) && defined(TARGET_ARM64)
// SIMD types are returned in vector regs on ARM64.
return (gtType == TYP_STRUCT) && !HasRetBufArg();
#elif FEATURE_MULTIREG_RET
return varTypeIsStruct(gtType) && !HasRetBufArg();
#else
if (varTypeIsSIMD(gtType))
{
return false;
}
#endif // FEATURE_HFA && TARGET_ARM64

if (!varTypeIsStruct(gtType) || HasRetBufArg())
{
return false;
}
// Now it is a struct that is returned in registers.
return GetReturnTypeDesc()->IsMultiRegRetType();
Copy link
Contributor Author

@sandreenko sandreenko May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CarolEidt I have changed that function and added GetReturnTypeDesc use here. The function is declared as const, so I had to add GetReturnTypeDesc() const version. With GetReturnTypeDesc I can distinguish structs that are returned in one register from multireg calls.

Non-const version is also needed, we sometimes do initialization as

        ReturnTypeDesc* retTypeDesc = callNode->GetReturnTypeDesc();
        retTypeDesc->InitializeLongReturnType(m_compiler);

I did not like that I had two identical implementations, but they are too small to do const_cast trick (and it is also not considered "right" for that nowadays, see https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#example-210). So I tried to create two new methods instead: call->InitializeLongReturnType() and call->InitializeStructReturnType() but it looked less clear after I implemented it.

Copy link
Contributor

@CarolEidt CarolEidt May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about this, and would not hold up this PR for it, but I think I prefer your alternate solution (having methods defined on the call node), or at least use different names (I guess I disagree with the C++ experts on that point).
If you leave them as-is, it would be worth a comment.

Copy link
Contributor

@CarolEidt CarolEidt May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making that change!

#else // !FEATURE_MULTIREG_RET
return false;
#endif
}
Expand Down
69 changes: 35 additions & 34 deletions src/coreclr/src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8898,55 +8898,56 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN
// Not allowed for FEATURE_CORCLR which is the only SKU available for System V OSs.
assert(!call->IsVarargs() && "varargs not allowed for System V OSs.");

// The return type will remain as the incoming struct type unless normalized to a
// single eightbyte return type below.
call->gtReturnType = call->gtType;

unsigned retRegCount = retTypeDesc->GetReturnRegCount();
if (retRegCount != 0)
if (retRegCount == 0)
{
// struct not returned in registers i.e returned via hiddden retbuf arg.
call->gtCallMoreFlags |= GTF_CALL_M_RETBUFFARG;
}
else if (retRegCount == 1)
{
if (retRegCount == 1)
if (!compDoOldStructRetyping())
{
// See if the struct size is smaller than the return
// type size...
if (retTypeDesc->IsEnclosingType())
return call;
}
// See if the struct size is smaller than the return
// type size...
if (retTypeDesc->IsEnclosingType())
{
// If we know for sure this call will remain a call,
// retype and return value via a suitable temp.
if ((!call->CanTailCall()) && (!call->IsInlineCandidate()))
{
// If we know for sure this call will remain a call,
// retype and return value via a suitable temp.
if ((!call->CanTailCall()) && (!call->IsInlineCandidate()))
{
call->gtReturnType = retTypeDesc->GetReturnRegType(0);
return impAssignSmallStructTypeToVar(call, retClsHnd);
}
call->gtReturnType = retTypeDesc->GetReturnRegType(0);
return impAssignSmallStructTypeToVar(call, retClsHnd);
}
else
{
// Return type is same size as struct, so we can
// simply retype the call.
call->gtReturnType = retTypeDesc->GetReturnRegType(0);
call->gtReturnType = call->gtType;
}
}
else
{
// must be a struct returned in two registers
assert(retRegCount == 2);

if ((!call->CanTailCall()) && (!call->IsInlineCandidate()))
{
// Force a call returning multi-reg struct to be always of the IR form
// tmp = call
//
// No need to assign a multi-reg struct to a local var if:
// - It is a tail call or
// - The call is marked for in-lining later
return impAssignMultiRegTypeToVar(call, retClsHnd);
}
// Return type is same size as struct, so we can
// simply retype the call.
call->gtReturnType = retTypeDesc->GetReturnRegType(0);
}
}
else
{
// struct not returned in registers i.e returned via hiddden retbuf arg.
call->gtCallMoreFlags |= GTF_CALL_M_RETBUFFARG;
// must be a struct returned in two registers
assert(retRegCount == 2);

if ((!call->CanTailCall()) && (!call->IsInlineCandidate()))
{
// Force a call returning multi-reg struct to be always of the IR form
// tmp = call
//
// No need to assign a multi-reg struct to a local var if:
// - It is a tail call or
// - The call is marked for in-lining later
return impAssignMultiRegTypeToVar(call, retClsHnd);
}
}

#else // not UNIX_AMD64_ABI
Expand Down