Skip to content

use stmt instead of Instruction in populate_def_use_map! #56201

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jumerckx
Copy link

fixes #56193

I imagine this maybe needs a test but not sure how to properly unit test this without creating a full end-to-end test using Base.code_ircode and so on.

@oscardssmith oscardssmith added bugfix This change fixes an existing bug compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Oct 16, 2024
@KristofferC
Copy link
Member

@aviatesk, could you maybe have a look at this?

@aviatesk
Copy link
Member

Yeah, I can take a look on this. Maybe tomorrow or so.

@LilithHafner
Copy link
Member

Bump

@willtebbutt
Copy link
Contributor

willtebbutt commented Feb 6, 2025

At least in my hands, the fix in this PR does indeed fix the problem, but it uncovers a separate issue at

# bail out early if there are no possibilities to refine the effects
if !any_refinable(sv)
return nothing
end

The problem appears to be the following: as a side-effect of calling ScanStmt, the first phase of the TwoPhaseDefUseMap (tpdum) associated to its sv field is performed during

stmt_inconsistent = scan_inconsistency!(inst, sv)

i.e. it counts ssa uses -- see here .

Returning nothing on line 912 causes the underlying scan! to stop traversing the IRCode. This early stopping behaviour means that the tpdum can fail to properly count the number of uses associated to each SSA.

If this happens, it means that when CC.populate_def_use_map! is called here it has an underestimate of the total number of SSA uses, and will have incorrect counts in its ssa_uses field. Since CC.populate_def_use_map! definitely does a full pass over all (connected?) SSAs (since its call to scan! never returns nothing), it errors at some point when it tries to write to an element of tpdum.data which is beyond the end of the array.

In any case, one way to prevent this from happening is simply to not bail out early as part of calling a ScanStmt.

It makes sense that fixing CC.populate_def_use_map! would uncover this problem -- the fact that it wasn't previously ever attempting to write to tpdum.data due to the bug means that this problem would never happen.

I'm really struggling to generate a minimal reproducer for this -- I found this as part of my work on Mooncake.jl, and the example from which I figured out this behaviour involves a lot of algorithmically generated code, so isn't amenable to making a minimal reproducer. Does someone who knows more than me about this think that they have an obvious way to produce an minimal reproducer from this description?

edit: (I've confirmed locally that just commenting out lines 910-913 solves the problem, and have yet to encounter any new problems resulting from this)

edit2: there appear to be two more related places where uses can be missed. The first is another bailing-out-early example:

return nothing

The second is that it looks like this bit of code assumes that the boundscheck argument to various things must be a constant, when in fact it looks like it can also be an SSA:
for i = 1:(length(stmt.args)-1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CC.populate_def_use_map! doesn't work as expected.
6 participants