Ssa: Replace phi-read references in VariableCapture with default use-use flow #19154
Conversation
aschackmull
added
the
no-change-note-required
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- shared/dataflow/codeql/dataflow/VariableCapture.qll: Language not supported
- shared/ssa/codeql/ssa/Ssa.qll: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| @@ -1,2 +0,0 @@ | |||
| identityLocalStep | |||
| | main.rs:442:9:442:20 | phi(default_name) | Node steps to itself | | |||
There was a problem hiding this comment.
I can see corresponding results - a big decrease in data flow inconsistencies - in the DCA run for Rust. ✨
The Swift DCA run shows an analysis slowdown, which is likely noise, but big enough I wouldn't mind a re-run to confirm nothing is wrong there. Other than than the Rust and Swift DCA runs LGTM.
There was a problem hiding this comment.
The swift analysis slowdowns appear to be mostly on the order of the corresponding stddev values, so I expect it to be noise. But I've started a re-run, so we get another round of numbers.
There was a problem hiding this comment.
Hmm, looks like the Swift slowdown is at least somewhat consistent.
There was a problem hiding this comment.
Although, it's not that consistent which projects exhibit slowdowns, so the results definitely are noisy.
There was a problem hiding this comment.
I cannot reproduce any perf issues locally.
There was a problem hiding this comment.
In my experience, the Swift DCA analysis timings are completely useless (wdyt @redsun82 ?)
There was a problem hiding this comment.
unfortunately it's hard to get consistent timings from macOS runners, so I think those can be ignored
There was a problem hiding this comment.
The slowdowns being in different projects is good evidence this is noise to me. 👍
4 participants
The first commit contains the primary change: The VariableCapture library is updated to use the DataFlowIntegration module instead of direct references to phi-read nodes. The nested SSA instantiation causes a little bit of caching, so I make sure to collapse those stages with parts of VariableCapture that would otherwise be duplicated across several stages. This ensures that we maintain the same number of cached stages (as the nested SSA already introduces a stage), and I think it also ought to be a mostly strict reduction of cross-stage DIL duplication.
The subsequent 2 commits are simple SSA cleanups.