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

JIT: fix value numbering to handle GT_NULLCHECK more generally#18942

Merged
AndyAyersMS merged 3 commits into
dotnet:masterfrom
AndyAyersMS:Fix18937
Jul 17, 2018
Merged

JIT: fix value numbering to handle GT_NULLCHECK more generally#18942
AndyAyersMS merged 3 commits into
dotnet:masterfrom
AndyAyersMS:Fix18937

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

With the advent of #18819 we may now see GT_NULLCHECK nodes with operands
that also can cause exceptions.

Handle this in value numbering.

Closes #18937.

With the advent of dotnet#18819 we may now see GT_NULLCHECK nodes with operands
that also can cause exceptions.

Handle this in value numbering.

Closes #18937.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

@briansull PTAL
cc @dotnet/jit-contrib

No diffs in the _il_relldobj_V.exe from before #18819.
Don't expect any other diffs; checking now.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@echesakov can you launch one or more of the Pri1 test legs that you saw failing?

@echesakov
Copy link
Copy Markdown

@dotnet-bot test CentOS7.1 x64 Build and Test
@dotnet-bot test Ubuntu x64 Build and Test
@dotnet-bot test Ubuntu arm Cross Checked normal Build and Test
@dotnet-bot test Windows_NT arm Cross Checked normal Build and Test

@AndyAyersMS
Copy link
Copy Markdown
Member Author

OSX failure looks like an infrastructure issue:

/Users/dotnet-bot/j/workspace/dotnet_coreclr/master/x64_checked_osx10.12_innerloop_prtest/Tools/dotnetcli/sdk/2.1.301/NuGet.targets(114,5): 
error : Failed to retrieve information about 'microsoft.dotnet.buildtools.coreclr' from remote source

will retry if everything else passes.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

No PMI diffs on x64 across all pri1 tests. No crossgen diffs for frameworks.

@dotnet-bot retest OSX10.12 x64 Checked Innerloop Build and Test

Comment thread src/jit/valuenum.cpp Outdated
vnStore->VNPairForFunc(TYP_REF, VNF_NullPtrExc,
vnStore->VNPNormVal(tree->gtOp.gtOp1->gtVNPair)));

excSet = vnStore->VNPExcSetUnion(excSet, vnStore->VNPExcVal(tree->gtOp.gtOp1->gtVNPair));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that we should declare a new local excSetBoth or similar instead of stomping on the old value of excSet

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, will fix.

Comment thread src/jit/valuenum.cpp Outdated
vnStore->VNPairForFunc(TYP_REF, VNF_NullPtrExc,
tree->gtOp.gtOp1->gtVNPair)));
break;
// Handle case where operand tree also may case exceptions.
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.

Typo: case --> cause

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

OSX testing now failing with some pal test errors.

Am going to push an update with changes from feedback and ignore OSX if it fails again.

@AndyAyersMS AndyAyersMS merged commit 2a4db3b into dotnet:master Jul 17, 2018
@AndyAyersMS AndyAyersMS deleted the Fix18937 branch July 17, 2018 01:48
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.

4 participants