Skip to content
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.

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

Fix VN of an ASG involving struct retyping. #42806

Closed
wants to merge 5 commits into from

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Sep 28, 2020

Fixes #42517, replaces logic from dotnet/coreclr#24482 .

Diffs are small as expected:
x64 crossgen - no diffs;
x64 pmi - +113 (1 improved, 3 regressed);
arm64 crossgen - +8 (1 regressed);
arm64 pmi - +40 (1 improved, 3 regressed);
and similar for x86.

An example diff (from arm64 win pmi):

N005 ( 10, 10) [000097] -A------R---              *  ASG       struct (copy)
N004 (  6,  7) [000096] n-----------              +--*  BLK       struct<StackFrame, 16>
N003 (  3,  5) [000095] ------------              |  \--*  ADDR      byref 
N002 (  3,  4) [000084] U------N----              |     \--*  LCL_FLD   struct V07 tmp3         ud:2->3[+16] Fseq[_frame]
N001 (  3,  2) [000093] -------N----              \--*  LCL_VAR   struct<StackFrame, 16> V11 tmp7         u:1 (last use)

--- "a/D:\\Sergey\\logs\\PushTag\\old.txt"
+++ "b/D:\\Sergey\\logs\\PushTag\\new.txt"
@@ -5,7 +5,7 @@ 
 N003 (  3,  5) [000095] ------------              |  \--*  ADDR      byref
 N002 (  3,  4) [000084] U------N----              |     \--*  LCL_FLD   struct V07 tmp3         ud:2->3[+16] Fseq[_frame]
 N001 (  3,  2) [000093] -------N----              \--*  LCL_VAR   struct<StackFrame, 16> V11 tmp7         u:1 (last use)

-N001 [000093]   LCL_VAR   V11 tmp7         u:1 (last use) => $388 {388}
+N001 [000093]   LCL_VAR   V11 tmp7         u:1 (last use) => $387 {387}
   VNApplySelectors:
     VNForHandle(_frame) is $1ca, fieldType is struct, size = 16
       AX2: $1ca != $1c8 ==> select([$342]store($1, $1c8, $80), $1ca) ==> select($1, $1ca).
@@ -19,13 +19,13 @@
 N001 [000093]   LCL_VAR   V11 tmp7         u:1 (last use) => $388 {388}
 N002 [000084]   LCL_FLD   V07 tmp3         ud:2->3[+16] Fseq[_frame] => <l:$252 {252}, c:$253 {253}>
     FieldSeq {_frame} is $214
 N003 [000095]   ADDR      => $185 {PtrToLoc($45, $214)}
-  VNApplySelectorsAssign:
-    VNForHandle(_frame) is $1ca, fieldType is struct
-    VNForMapStore($342, $1ca, $388):struct returns $500 {$342[$1ca := $388]}
-  VNApplySelectorsAssign:
-    VNForHandle(_frame) is $1ca, fieldType is struct
-    VNForMapStore($342, $1ca, $388):struct returns $500 {$342[$1ca := $388]}
-Tree [000097] assigned VN to local var V07/3: $500 {$342[$1ca := $388]}
+Generate a unique VN for  on V11 because of struct reinterpretation
+
+
+
+
+
+Tree [000097] assigned VN to local var V07/3: new uniq $388 {388}
 N005 [000097]   ASG       => $VN.Void

 ***** BB03, STMT00021(after)
@@ -33,4 +33,4 @@ N005 ( 10, 10) [000097] -A------R---              *  ASG       struct (copy) $VN
 N004 (  6,  7) [000096] n-----------              +--*  BLK       struct<StackFrame, 16>
 N003 (  3,  5) [000095] ------------              |  \--*  ADDR      byref  $185
 N002 (  3,  4) [000084] U------N----              |     \--*  LCL_FLD   struct V07 tmp3         ud:2->3[+16] Fseq[_frame] <l:$252, c:$253>
-N001 (  3,  2) [000093] -------N----              \--*  LCL_VAR   struct<StackFrame, 16> V11 tmp7         u:1 (last use) $388
+N001 (  3,  2) [000093] -------N----              \--*  LCL_VAR   struct<StackFrame, 16> V11 tmp7         u:1 (last use) $387

it looks like this change is valid because if 000084 type was not <StackFrame, 16> it could confuse VN optimizations. For example, if the type was NotAStackFrame and we access its field using an VN from StackField we will get the same bug as this PR is fixing.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 28, 2020
@sandreenko sandreenko changed the title Fix VN of an unsafe cast Fix VN of an ASG involving struct retyping. Sep 28, 2020
"reinterpretation\n",
addrChild->AsLclVarCommon()->GetLclNum());

varDsc->lvOverlappingFields = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there were no overlapping fields there, the flag was used to mark the local var as "bad" for optimizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reinterpreting a stuct of one type as a struct of a different type, essentially is the same as a C++ union where any and all fields of the structs overlap with each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

So such as case is always "bad for optimizations"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a C++ union because in this case we copy memory, so we don't address the same bytes using different handles.
What you are talking about in our model is represented as "address exposed" in my understanding.

@@ -6687,7 +6683,16 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
{
var_types indType = lclVarTree->TypeGet();
ValueNum fieldSeqVN = srcAddrFuncApp.m_args[0];
// It could return a sequence that ends with a pseudo fields that does not make sense for me.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some static vars we are getting field seq that ends with a pseudo field, not sure why. It could be another issue.
x86.failure.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am looking for a better way to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pseudo fields are things like the first element of an array

@sandreenko
Copy link
Contributor Author

PTAL @briansull, cc @dotnet/jit-contrib

@sandreenko
Copy link
Contributor Author

I am trying to understand some VN principles and can't find documentation, the idea is described in JIT Architecture Plan (2009), but do we have description of the actual implementation?

@briansull
Copy link
Contributor

These field maps was one of the last things added by Dave Detlefs before he left.
I don't know of additional documentation on this.

@sandreenko sandreenko closed this Dec 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR 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.

Test failure: System.Buffers.Tests.ArrayBufferWriterTests_Char.Advance
2 participants