-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
b13146f
to
bf7a400
Compare
Is it ready for review? |
Hmm, looks like it was. I only now notice that it has conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I will merge it when you fix the merge conflicts.
|
||
assert(lpIterTree); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this assert was deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it odd to assert that a pointer is non-null when you're going to dereference it on the next line anyway. Such asserts are typically used when you store the pointer for later use and want to avoid wasting your time trying to figure out how did the null get there.
src/jit/rangecheck.cpp
Outdated
@@ -875,29 +859,15 @@ Range RangeCheck::ComputeRangeForLocalDef(BasicBlock* block, | |||
JITDUMP("----------------------------------------------------\n"); | |||
} | |||
#endif | |||
switch (asg->OperGet()) | |||
Range range = GetRange(asgBlock, asg->gtGetOp2(), monotonic DEBUGARG(indent)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add an assert that asg->OperIs(GT_ASG)
just to make my paranoia calm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit redundant but I'll add it. Hopefully GT_ASG
will disappear one day and take the assert with it :)
cc @dotnet/jit-contrib |
Assert added, conflicts fixed. Turns out that in one case I managed to update the same comment in another PR and used different wording. Erm. |
#19613 (comment) |
@dotnet-bot test this please |
close/reopen has helped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM with one comment suggestion
src/jit/compiler.h
Outdated
@@ -4098,8 +4098,8 @@ class Compiler | |||
|
|||
void fgInterBlockLocalVarLiveness(); | |||
|
|||
// The presence of "x op= y" operations presents some difficulties for SSA: this is both a use of some SSA name of | |||
// "x", and a def of a new SSA name for "x". The tree only has one local variable for "x", so it has to choose | |||
// The presence of struct field assignment presents some difficulties for SSA: this is both a use of some SSA name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are cases of partial assignment where even a primitive type lclVar can have a partial assignment (obviously not safe or verifiable code, of course). Perhaps this would be better as "The presence of partial assignments, e.g. a struct field assignment or a GT_LCL_FLD assignment of a portion of a local, presents ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, *((byte*)&i) = …
does produce a partial def. It's a byte LCL_FLD
so it blocks enregistration but it's not address exposed so it does appear in SSA.
I've update this and another SsaBuilder comment to only say "partial definition" (seems to used more commonly than "partial assignment") and extended the GTF_VAR_USEASG
comment to say what a partial definition is:
#define GTF_VAR_USEASG 0x40000000 // GT_LCL_VAR -- this is a partial definition, a use of the previous definition is implied
// A partial definition usually occurs when a struct field is assigned to (s.f = ...) or
// when a scalar typed variable is assigned to via a narrow store (*((byte*)&i) = ...).
No description provided.