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

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
Add an additional check that ReturnTypeDesc is initialized.
  • Loading branch information
Sergey Andreenko committed May 14, 2020
commit 81dda67eb95b88ae6b4e658d37f7650929f0435d
3 changes: 2 additions & 1 deletion src/coreclr/src/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3680,6 +3680,7 @@ struct ReturnTypeDesc
}
else
{
assert(m_inited);
return ((m_regType[0] != TYP_UNKNOWN) && (m_regType[1] != TYP_UNKNOWN));
}
}
Expand Down Expand Up @@ -4256,7 +4257,7 @@ struct GenTreeCall final : public GenTree
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
#endif // !FEATURE_MULTIREG_RET
}

// Returns true if VM has flagged this method as CORINFO_FLG_PINVOKE.
Expand Down