Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix condition for calling EvalArgsToTemps#22791

Merged
CarolEidt merged 1 commit into
dotnet:masterfrom
CarolEidt:Fix19256
Mar 4, 2019
Merged

Fix condition for calling EvalArgsToTemps#22791
CarolEidt merged 1 commit into
dotnet:masterfrom
CarolEidt:Fix19256

Conversation

@CarolEidt
Copy link
Copy Markdown

fgArgInfo::ArgsComplete() checks for additional conditions requiring temps that are not checked for in the body of fgMorphArgs(). However, if there are no register args, it won't be called.

Fix #19256

@CarolEidt
Copy link
Copy Markdown
Author

The reason that this issue didn't exist prior to #18358 was that the body of fgMorphArgs was overly aggressive in making copies, and it used the criteria of any copies having been made to decide whether to call EvalArgsToTemps - this case doesn't need a copy for the reasons checked for in the body of fgMorphArgs, but it does need a copy due to side-effects of down-stream args. This is checked for in fgArgInfo::ArgsComplete() but that wasn't being called unless there were register arguments or, prior to #18358, if any structs had been copied.

Comment thread src/jit/morph.cpp
#ifdef DEBUG
if (verbose)
{
JITDUMP("ArgTable for %d.%s after fgMorphArgs:\n", call->gtTreeID, GenTree::OpName(call->gtOper));
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This, and the similar change below, are because it's hard to figure out which argtable dump is for which calls when there are nested calls.

Copy link
Copy Markdown
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Looks good. Suggest one minor comment edit.

Comment thread src/jit/morph.cpp
// all cases of fgMakeOutgoingStructArgCopy() being called. hasStackArgCopy
// is added to make sure to call EvalArgsToTemp.
if (!reMorphing && (call->fgArgInfo->HasRegArgs()))
if (!reMorphing && (call->fgArgInfo->HasRegArgs() || call->fgArgInfo->NeedsTemps()))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can probably just delete the second part of this comment.

Copy link
Copy Markdown
Author

@CarolEidt CarolEidt Feb 22, 2019

Choose a reason for hiding this comment

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

Yes; I think I intended to do that after changing the first part, but apparently forgot.

@CarolEidt
Copy link
Copy Markdown
Author

test OSX10.12 x64 Checked Innerloop Build and Test
test Ubuntu arm Cross Checked Innerloop Build and Test
test Ubuntu arm Cross Checked crossgen_comparison Build and Test
test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
test Ubuntu arm Cross Release crossgen_comparison Build and Test
test Windows_NT arm Cross Checked Innerloop Build and Test
test Windows_NT arm64 Cross Checked Innerloop Build and Test
test Windows_NT x64 Checked CoreFX Tests
test Windows_NT x64 Checked Innerloop Build and Test
test Windows_NT x64 Formatting
test Windows_NT x64 Release CoreFX Tests
test Windows_NT x86 Checked Innerloop Build and Test
test Windows_NT x86 Release Innerloop Build and Test

@CarolEidt
Copy link
Copy Markdown
Author

@dotnet-bot retest this please

`fgArgInfo::ArgsComplete()` checks for additional conditions requiring temps that are not checked for in the body of `fgMorphArgs()`. However, if there are no register args, it won't be called.

Fix #19256
@CarolEidt
Copy link
Copy Markdown
Author

test OSX10.12 x64 Checked Innerloop Build and Test

@CarolEidt
Copy link
Copy Markdown
Author

test Ubuntu x64 Checked CoreFX Test
test Windows_NT arm Cross Checked Innerloop Build and Test
test Windows_NT arm64 Cross Checked Innerloop Build and Test
test Windows_NT x64 Checked CoreFX Test
test Windows_NT x64 Checked Innerloop Build and Test
test Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
test Windows_NT x64 Formatting
test Windows_NT x64 Release CoreFX Tests
test Windows_NT x86 Checked Innerloop Build and Test
test Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
test Windows_NT x86 Release Innerloop Build and Test
test Ubuntu arm Cross Checked Innerloop Build and Test
test Ubuntu arm Cross Checked crossgen_comparison Build and Test
test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
test Ubuntu arm Cross Release crossgen_comparison Build and Test

@CarolEidt
Copy link
Copy Markdown
Author

test Windows_NT x64 Release CoreFX Tests
test OSX10.12 x64 Checked Innerloop Build and Test
test Ubuntu x64 Checked CoreFX Test
test Windows_NT arm64 Cross Checked Innerloop Build and Test
test Windows_NT x64 Checked CoreFX Tests

@CarolEidt
Copy link
Copy Markdown
Author

test Ubuntu x64 Checked CoreFX Tests

@CarolEidt CarolEidt merged commit fd3afca into dotnet:master Mar 4, 2019
@CarolEidt CarolEidt deleted the Fix19256 branch March 4, 2019 16:54
CarolEidt added a commit to CarolEidt/coreclr that referenced this pull request Mar 8, 2019
In dotnet#22791 I was creating struct assignments with COMMAs on the rhs, but that isn't handled downstream.

Fix #23059
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Fix condition for calling EvalArgsToTemps

Commit migrated from dotnet/coreclr@fd3afca
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
In dotnet/coreclr#22791 I was creating struct assignments with COMMAs on the rhs, but that isn't handled downstream.

Fix dotnet/coreclr#23059


Commit migrated from dotnet/coreclr@384eefb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants