Skip to content
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

fix #52531, fix the effects modeling of QuoteNode #52548

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Conversation

aviatesk
Copy link
Member

What observed in #52531 is that QuoteNode can embed global variables that users can modify. Therefore, when dealing with QuoteNode, it's necessary to taint its :inaccessiblememonly just like we do for GlobalRef.

What observed in #52531 is that `QuoteNode` can embed global variables
that users can modify. Therefore, when dealing with `QuoteNode`, it's
necessary to taint its `:inaccessiblememonly` just like we do for
`GlobalRef`.
@aviatesk aviatesk added compiler:effects effect analysis backport 1.10 Change should be backported to the 1.10 release labels Dec 15, 2023
@vchuravy
Copy link
Member

I think in the current design this is the right solution.

Coming from LLVM our modelling of inaccessiblememonly is a bit odd. In LLVM inaccessiblememonly/argmemonly are properties of functions (declaration and calls) and are with respect to their arguments.

So Expr(:call, :getfield, obj, :x) could never be inaccesssiblememonly since it always access memory through it's argument.

It took me a while to understand that in Julia these are like the other effects modeled with respect to the containing/parent function.

E.g. inaccessiblememonly -> consistent (logical implies/conditional) If the parent function is proven to be inaccessiblememonly then getfield is consistent.
It may make sense to separate the memory effects out from other effects and handle them separate for #36832 I eventually need to ask the question: "What are the memory effects of this instruction" and there I need inaccessiblememonly and argmemonly in the LLVM sense. I would need to be able to answer the query: What memory semantics does this call have (not what memory semantics does this call imply for its parent). That's also why ModRef separates between write/read and then different scopes: global/task/arguments/inaccessible.

To infer the function global effects we would then need to ask the question "where did the memory come from".

@aviatesk
Copy link
Member Author

Although I'm not completely certain about the exact interpretation of :inaccessiblememonly in the LLVM sense, it seems feasible to convert our representation to match LLVM's? For instance, a getfield call would be considered inaccessible in LLVM if :inaccessiblememonly = ALWAYS_TRUE, global if ALWAYS_FALSE, and argument if INACCESSIBLEMEM_OR_ARGMEMONLY (or am I missing your point?). Julia's :inaccessiblememonly doesn't currently cover read/write or task, but adding these features shouldn't be too challenging.

So Expr(:call, :getfield, obj, :x) could never be inaccesssiblememonly since it always access memory through it's argument.

Hmm really? As far as I understand LLVM's :inaccessiblememonly is all about mutable memory and that it's interpreted as :inaccessiblememonly when arguments are mutation-free, which is what we do on Julia-level analysis for getfield call.

@vchuravy
Copy link
Member

(or am I missing your point?

I think so. My point is that in LLVM the memory attribute is with reference to the called function (not within the context of the calling function).

As far as I understand LLVM's :inaccessiblememonly is all about mutable memory

No. LLVM doesn't make the distinction between mutable/immutable memory that we do. So there is no refinement for immutable memory (which also wouldn't be inaccessiblememonly, but rather would be readnone)

InaccessibleMemOnly: This attribute indicates that the function may only access memory that is not accessible by the program/IR being compiled. This is a weaker form of ReadNone.

So it says this function does read memory, but none that is reachable from the caller (also see https://groups.google.com/g/llvm-dev/c/1ptivB6H6_c/m/oxLBU4tqAAAJ)

@gbaraldi
Copy link
Member

Right, memory attributes with respect to arguments don't translate to the parent (they do for accessing globals of course).

@KristofferC KristofferC merged commit e207c10 into master Dec 15, 2023
7 of 9 checks passed
@KristofferC KristofferC deleted the avi/52536 branch December 15, 2023 20:29
@KristofferC
Copy link
Member

@aviatesk, can you push a backported commit of this to the backport 1.10 branch?

aviatesk added a commit that referenced this pull request Dec 16, 2023
What observed in #52531 is that `QuoteNode` can embed global variables
that users can modify. Therefore, when dealing with `QuoteNode`, it's
necessary to taint its `:inaccessiblememonly` just like we do for
`GlobalRef`.

- fixes #52531
- replaces #52536
@aviatesk aviatesk removed the backport 1.10 Change should be backported to the 1.10 release label Dec 16, 2023
@aviatesk
Copy link
Member Author

@aviatesk, can you push a backported commit of this to the backport 1.10 branch?

Done.

@aviatesk aviatesk mentioned this pull request Dec 16, 2023
17 tasks
KristofferC added a commit that referenced this pull request Dec 17, 2023
Backported PRs:
- [x] #51234 <!-- Fix getfield codegen for tuple inputs and unknown
symbol fields. -->
- [x] #52170 <!-- fix invalidations related to `ismutable` -->
- [x] #52342 <!-- Add single-term multiplication for `AbstractQ` on
v1.10 and above -->
- [x] #52333 <!-- bugfix for dot of Hermitian{noncommutative} -->
- [x] #52407 <!-- channels: fix memory ordering violation in iterate -->
- [x] #52405 <!-- Bump LLVM to 15.0.7+10 to fix GC issue -->
- [x] #52441 <!-- Remove `Pkg` dependency from `SuiteSparse_jll` -->
- [x] #52367 <!-- docs: add notes about scratchspaces in depot -->
- [x] #52456 <!-- Make `jl_write_coverage_data` dllexported again -->
- [x] #52294 <!-- GC scheduler refinements -->
- [x] #52359 <!-- make custom log macros work -->
- [x] #52548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mutable let block variable treated as immutable in Julia 1.10
4 participants