fix internal IR corruption from use of a global #39540
Conversation
|
Can we come up with a form that is always legal to have here? Requiring the type lifting pass to be run seems like a code smell. |
base/compiler/ssair/passes.jl
Outdated
| compact.result[idx][:line], true) | ||
| compact.ssa_rename[compact.idx-1] = pi | ||
| Y = stmt.args[2] | ||
| if !isa(Y, GlobalRef) |
There was a problem hiding this comment.
Why doesn't GlobalRef work here? Can you explain that in a comment or in the commit message?
There was a problem hiding this comment.
GlobalRef is not pure, so it is invalid to duplicate it
There was a problem hiding this comment.
Wasn't the whole point of #36450 to change that? What non-purity remains?
There was a problem hiding this comment.
They can also still be non-constant
There was a problem hiding this comment.
If there is a data race you mean? In that case, what if instead of not doing the canonicalization, we did:
%a = GlobalRef(...)
typassert(%a, T)
PiNode(%a, T)
|
I'm not sure. This part of the code needs more documentation. The issue is we're inserting a use of an SSAValue, whose purpose (later) is to tell us is to let us track backwards (through PhiNodes) to discover if it was legal dynamically |
|
What prevents us from just making |
|
I don't think it ever existed (I tried to search back a little, but didn't see anything). |
I'm not sure I agree with that. It's a dynamic check on the definedness on that branch. I guess the issue is that we don't have the capacity to track this in codegen and if we did, it would just look like |
| @@ -147,6 +148,7 @@ function fixemup!(cond, rename, ir::IRCode, ci::CodeInfo, idx::Int, @nospecializ | |||
| return true | |||
| end | |||
| end | |||
| # temporarily corrupt the isdefined node. type_lift_pass! will fix it | |||
There was a problem hiding this comment.
Should this be a separate Expr head then?
There was a problem hiding this comment.
Probably would be more accurate? But possibly annoying to implement? Dunno. I was going for the minimal fix
These ops are not actually legal (they're semantically invalid), but we temporarily use them to carry information between passes which needs to then remove them correctly. Fixes #39508
|
I've dropped the objectionable commit and created #39893 for the better alternative of it. This should be ready to merge |
|
Agreed. I think the main challenge is that we need a concretization pass at some point to split maybe-undef variables, and it either looks a lot like this pass, but could be run separately / very early, or it just is this pass. |
Fixes #39508