Skip to content

Fix fgValueNumberArrIndexVal for wide reads - #58309

Merged
EgorBo merged 4 commits into
dotnet:mainfrom
EgorBo:fix-ind-vn-issue
Aug 31, 2021
Merged

Fix fgValueNumberArrIndexVal for wide reads#58309
EgorBo merged 4 commits into
dotnet:mainfrom
EgorBo:fix-ind-vn-issue

Conversation

@EgorBo

@EgorBoEgorBo commented Aug 28, 2021

Copy link
Copy Markdown
Member

Fixes#57914

Minimal repro:

privatestaticboolTest1(){byte[]array=newbyte[2];array[0]=1;array[1]=2;bytea1=Unsafe.ReadUnaligned<byte>(refarray[0]);shorta2=Unsafe.ReadUnaligned<short>(refarray[0]);array[1]=42;shorta3=Unsafe.ReadUnaligned<short>(refarray[0]);returna1==a2&&a1==a3;// returns True currently}

a1, a2 and a3 all have different values here with the same liberal VN resulting Test1 to return true.

a1:

\--* IND byte <l:$42, c:$401>
\--* ADD byref $300
+--* LCL_VAR ref V04 tmp1 u:2 $240
\--* CNS_INT long 16 Fseq[#FirstElem] $181

a2:

\--* IND short <l:$500, c:$5c1>
\--* LCL_VAR byref V07 tmp4 u:2 (last use) $300

($500 here is $42 with NullRefExc)

a3:

\--* IND byte <l:$42, c:$402>
\--* ADD byref $300
+--* LCL_VAR ref V04 tmp1 u:2 (last use) $240
\--* CNS_INT long 16 Fseq[#FirstElem] $181

The fix is quite conservative - it treats such array element loads as "non-proper" and assign a unique VN.
SuperPMI diffs are empty.

PS: Doesn't reproduce on .NET 5.0

/cc @dotnet/jit-contrib

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 28, 2021
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Minimal repro:

privatestaticboolTest1(){byte[]array=newbyte[2];array[0]=1;array[1]=2;bytea1=Unsafe.ReadUnaligned<byte>(refarray[0]);shorta2=Unsafe.ReadUnaligned<short>(refarray[0]);array[1]=42;shorta3=Unsafe.ReadUnaligned<short>(refarray[0]);returna1==a2&&a1==a3;}

a1, a2 and a3 all have different values here with the same liberal VN resulting Test1 to return true.

The fix is quite conservative - it treats such array element loads as "non-proper" and assign a unique VN.
Ideas for a less conservative fix are welcomed 🙂

PS: Doesn't reproduce on .NET 5.0

/cc @dotnet/jit-contrib

Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@SingleAccretion

SingleAccretion commented Aug 28, 2021

Copy link
Copy Markdown
Contributor

Heh, so I always wondered if it is possible to make VN use CASTs where it ought to use BITCASTs. Turns out it is very possible.

privatestaticfloatBitcast(){int[]array=newint[2];array[0]=1;returnUnsafe.ReadUnaligned<float>(refUnsafe.As<int,byte>(refarray[0]));}
 wholeElem $41 is MapSelect(hAtArrTypeAtArr($340), ind=$142).
VNForCastOper(float) is $42VNForCast($41, $42) returns $440 {FltCns[1.000000]}
selectedElem is $440 after applying selectors.

But that is a separate issue (filed as #58312).

@EgorBoEgorBo added this to the 6.0.0 milestone Aug 28, 2021

@sandreenkosandreenko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@briansullbriansull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks Good,

@EgorBo
EgorBo merged commit 098dc8b into dotnet:mainAug 31, 2021
@EgorBo

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1185769084

@ghostghost locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] inliningVars.cmd fails

4 participants

@EgorBo@SingleAccretion@briansull@sandreenko