-
Notifications
You must be signed in to change notification settings - Fork 457
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 the incorrect merge logic for back edges for GlobalFlowStateAnalysis #6029
Conversation
Fixes dotnet#6015 `GlobalFlowStateAnalysisValueSet.Unset` is used to represent unanalyzed flow state, `GlobalFlowStateAnalysisValueSet.Empty` is used to represent analyzed flow state with no analysis values. We initialize the flow states for the entry basic block of the CFG with `Unset` value, but never replace the `Unset` with `Empty` for subsequent passes. This in turn leads to incorrect merge when a `Known` value flows from the back edge to the loop start and ends up overriding the `Unset` value instead of being overridden by the `Empty` value. Now we correctly perform this merge logic. I verified that prior to this fix, the added unit test hang due to this issue. I also verified that building the github repo cited in the feedback ticket with platform compat analyzer also leads to a hang before this fix. Both these repros are fixed after this PR.
{ | ||
return value1; | ||
return GlobalFlowStateAnalysisValueSet.Empty; |
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.
It's not entirely clear to me why you always set Empty in the case of merge.
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.
@Evangelink This matches the intended merge logic also documented in
Lines 7 to 13 in 0e7f40d
/// <summary> | |
/// Unset value set. | |
/// This is needed along with Empty to ensure the following merge results: | |
/// Unset + Known = Known | |
/// Empty + Known = Empty | |
/// </summary> | |
Unset, |
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.
Didn't read the doc... sorry :)
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.
Thanks for the fix!
@mavasani JFYI: the build failures with NuGet errors: NU1504, NU1505 I can also see locally |
@jmarolf is this a known issue? |
@mavasani @buyaa-n Functionality for that on NuGet-side was implemented in NuGet/NuGet.Client#4484. I'm fixing the error in #6032 |
let me take a look |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## main #6029 +/- ##
==========================================
- Coverage 96.03% 96.03% -0.01%
==========================================
Files 1338 1338
Lines 307164 307197 +33
Branches 9787 9788 +1
==========================================
+ Hits 294978 295007 +29
- Misses 9809 9812 +3
- Partials 2377 2378 +1 |
Fixes #6015
GlobalFlowStateAnalysisValueSet.Unset
is used to represent unanalyzed flow state,GlobalFlowStateAnalysisValueSet.Empty
is used to represent analyzed flow state with no analysis values. We initialize the flow states for the entry basic block of the CFG withUnset
value, but never replace theUnset
withEmpty
for subsequent passes. This in turn leads to incorrect merge when aKnown
value flows from the back edge to the loop start and ends up overriding theUnset
value instead of being overridden by theEmpty
value. Now we correctly perform this merge logic.I verified that prior to this fix, the added unit test hang due to this issue. I also verified that building the github repo cited in the feedback ticket with platform compat analyzer also leads to a hang before this fix. Both these repros are fixed after this PR.