Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
inference: inter-procedural conditional constraint back-propagation #38905
inference: inter-procedural conditional constraint back-propagation #38905
Changes from all commits
9be2edb
cf634a8
36e7e8a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
@vtjnash should I revert back to the previous
ismissing
/isnothing
definitions ?I believe the less method definitions the better, because we can keep to use
InterConditional
logic as far as there're ≦3 methods even after a package defines new methods for them.xref: https://github.com/JuliaLang/julia/pull/38905/files#r552878124
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 may be fixable, and highlights a complex oddity of how
Conditional
behaves in the lattice. The problem is this PR doesn't scan the vtypes list to realize the type ofa
is actually constrained in BB#3, so it over-approximates the true return type to be Bool. The true result type in BB#2 isConditional(:a@2, Int, Int)
. Along with the result type in BB#3 beingConditional(:a@2, Union{}, Any)
. Thus the correctly computed return type should have beenInterConditional(:a@2, Int, Any)
after the tmerge of those two.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.
yes, I think we can change our local inference logic so that we can annotate
Conditional
for a statement (e.g.%5
) with considering where it comes from (e.g.%1
), but I didn't try to tackle that in this PR because I felt the changes can be huge.Maybe we can merge this PR with only introducing
InterConditional
and leave this as a future work (in a separate PR) ?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.
The change should be pretty simple, though probably doesn't need to be in this PR: if the inferred result is <: Bool, then it is actually Conditional for the entire VarTable
s[pc][1:nargs]
. The annoying part then is that InterConditional can only represent exactly one of those args, so you may have to just pick the first interesting one. (conceptually, this is a also a generalizable observation on what Conditional represents: either it's an alternative VarTable based on a particular value comparison result—or the main VarTables
is a Conditional on the program state)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.
Okay, with the following diff, we can back-propagate constraints in cases like
ispositive
and also some cases likethe original definitions of
isnothing
andismissing
(it looks dirty, though):But I couldn't succeed to bootstrap with this (with
fatal error in type inference (type bound)
error)..Do you have any pointer about where to fix ?
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.
On closer inspection, I think you're missing
bestguess_to_interprocedural
on some of the uses ofbestguess
. While you could grep each usage, I think it might be better to consolidate it here: (I thought the new assert should have caught that)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.
Okay, dug deeper and found it is due to missing handling of Vararg expansion in this PR. Here's a reduced repro. Note how the
x...
turned into ax::Tuple{Symbol}
type description (which will fail at runtime, because actually we know thatx::Symbol
there)(was hoping to be able to explain simply how I found this, but ended up being a bit convoluted. Mostly I ran under
rr
until exit, then ran backwards tobreak jl_throw
, then back towatch
on the value of $rip, then back toeval_function
a couple times, until I could see the printing ofp jl_(src)
for the correctp jl_(li)
function. I searched this for "fatal" and sawTuple{Int}
, which I could tell was bad, so then it was "just" a matter of guessing different IR inputs until one propagated the type wrong)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 so much for your help !
Unfortunately I'm on macOS and it seems to me that debugging w/ gdb is hard on that platform (rr seems even unavailable ...).
I'll try to get Linux or docker environment to develop Julia, but for now I'd like to move forward this PR.
12e1e79 enhances this PR by eagerly converting boolean conditions into
(Inter)Conditional
s following your idea.Now I succeed in bootstrapping but it seems that there're few inference accuracy regressions.
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.
Okay I just fixed the inference accuracy regression; there're some code that make good use of
Const(val::Bool)
to improve inference quality (e.g.ifelse
tfunc), but now we turnConst(val::Bool)
return type intoConditional
, and so we need to respect its constant-ness to keep accuracy.As for
ifelse
one, I fixed that in fe6accd, but there may be other points that should be tweaked.Once we've done all of them, I think this PR should be good to go.
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.
Yes, on macOS, lldb is better, but setting up a VM just to run
rr
in linux is also great (Can you tellrr
is one of my favorite magic tools?)I think we also might want to stick with using
Const(::Bool)
as the result object when the new type doesn't refine the old type of the Argument (e.g. except when⊑(s[pc][i], s[1][i])
), then choose later (inabstract_call_gf_by_type
) to form a tighter bound if union-splitting withnapplicable > 1
. Note, you can examinefieldtype(sig, n)
to find out the type of the nth argument.