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

mark julia.write_barrier readonly on argument #39742

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Conversation

vchuravy
Copy link
Member

While it is marked InaccessibleMemOnly already, that seems to still be only partially implemented in LLVM.
IIUC readonly on the argument is correct since write_barrier will escape the pointer but not write to it.

@wsmoses and I noticed this when looking at IR for Enzyme.

@vchuravy vchuravy added the compiler:codegen Generation of LLVM IR and native code label Feb 18, 2021
@vchuravy vchuravy requested a review from Keno February 18, 2021 21:33
@Keno
Copy link
Member

Keno commented Feb 18, 2021

Hmm, this is tricky, and might require some LLVM IR reference language lawyering. The write barrier does twiddle bits in the object header. However, we treat those as inaccessible to the program and always mask them out on access. The IR reference says "If a readonly function writes memory visible to the program, or has other side-effects, the behavior is undefined. If a function writes to a readonly pointer argument, the behavior is undefined.". I think because of that we are technically in the clear since this memory isn't supposed to be "visible to the program"

@vchuravy vchuravy added the needs nanosoldier run This PR should have benchmarks run on it label Feb 20, 2021
@vchuravy
Copy link
Member Author

vchuravy commented Mar 3, 2021

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

@vchuravy
Copy link
Member Author

vchuravy commented May 7, 2021

@Keno ok if I land this? Well I need to look into the performance regression first

@Keno
Copy link
Member

Keno commented May 7, 2021

Yeah, why not

@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed needs nanosoldier run This PR should have benchmarks run on it labels Sep 14, 2021
@DilumAluthge DilumAluthge merged commit 3794f9a into master Sep 15, 2021
@DilumAluthge DilumAluthge deleted the vc/wb_readonly branch September 15, 2021 06:04
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 15, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants