Make SROA pass more aggressive + make SSA use counting more accurate #44557
Conversation
| isa(val, SSAValue) && (compact.used_ssas[val.id] += 1) | ||
| return_value = SSAValue(idx′) | ||
| inline_compact[idx′] = val |
There was a problem hiding this comment.
inline_compact[idx′] = val calls count_added_node!, which increments inline_compact.used_ssas, which === compact.used_ssas, so incrementing here is incorrect
| for i = 1:length(pn.values) | ||
| isassigned(pn.values, i) || continue | ||
| v = pn.values[i] | ||
| if isa(v, SSAValue) | ||
| compact.used_ssas[v.id] += 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
Here, if length(pn.edges) != 1, we call insert_node_here! on NewInstruction(pn..., which does this same incrementing. In the case where we just return pn.values[1], I'm not sure if there are paths that result in this getting counted twice, but we can at least make this change for now
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
| compact[idx] = nothing | ||
| compact[idx] = form_new_preserves(stmt, preserved, new_preserves) |
There was a problem hiding this comment.
Since compact[idx] === stmt, we first need to first set compact[idx] to nothing, because in-place modification can lead to miscounts by setindex.
| compact[leaf] = nothing | ||
| stmt.args[argidx] = lifted | ||
| compact[leaf] = stmt |
There was a problem hiding this comment.
Here we forgot to insert, but we also must use the compact[i] = nothing pattern to avoid miscounting
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Looks like most of the failures have the same backtrace, so probably look at those first and then maybe look at |
|
This should pass all CI, and most PkgEval, still working on the issue with Transducers though |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Some of those failures are likely to be real. Let's re-run to make sure we get rid of any stochastic failures, but afterwards, we probably need to go through and investigate the failures: @nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
|
|
@nanosoldier |
|
This needed some minor fixes that will certainly mean the current PkgEval will fail, so rebasing, pushing, and starting a new one to run over night. |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Ugh, yeah, some of these are definitely real. Looking into |
|
The one in |
|
Yeah, pretty sure those just need |
|
Yes, although it's interesting that this function is called with no leaves whatsoever. I'd need to look at the code again to think about whether that makes sense or not. |
|
@nanosoldier |
|
Still investigating others, but pretty hopeful they are stochastic considering none of them were in the previous run |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
ianatol
force-pushed
the
kf/moresroa
branch
2 times, most recently
from
465af1c to
5626427
Compare
This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this.
This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this.
…52338) This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this. --------- Co-authored-by: Shuhei Kadowaki <[email protected]>
Rebase of #44494 + removal of a couple times where we miscount
compact.used_ssas