codegen: handle dead code with unsafe_store of FCA pointers #50164
Conversation
|
Can you add the tests from #50244 |
|
Those tests are either not legal or already pass on master |
|
Cool, glad to see a fix coming up! :) |
|
Can you explain what's wrong with the tests in #50244? This first one seems legal from what I understand: # issue #50243
struct ImmutableNonIsBits
v::Vector{Int}
end
const ref50243 = Ref(ImmutableNonIsBits([1,2,3]))
load50243() = unsafe_load(Ptr{ImmutableNonIsBits}(pointer_from_objref(ref50243)))
sum50243() = sum(load50243().v) # (Do something with v to prevent the compiler from eliding)
@test load50243() === ref50243[]
@test sum50243() === 6
@test @allocated(sum50243()) === 0Is this fundamentally different than doing the same trick with an isbits type, like an NTuple? And i think this second one is possibly more illegal, but i'm not 100% clear why. const ref50243 = Ref(ImmutableNonIsBits([1,2,3]))
store50243!(v2) = unsafe_store!(Ptr{ImmutableNonIsBits}(pointer_from_objref(ref50243)), v2)
new50243 = ImmutableNonIsBits([1])
store50243!(new50243)
@test ref50243[] === new50243
@test @allocated(store50243!(ref50243[])) === 0... But neither of them pass on master - what makes you say that? |
|
The first tests pass on master for me, except for the allocated bytes test which is not semantically guaranteed. The |
|
Thanks, makes sense.
^ Yeah, this is specifically what I was trying to test. Even though it's not semantically guaranteed, I assume that it's something we want to preserve in our implementation of julia, which is why I thought it might be good to test. But maybe the goal with the test suite is that it should pass under any implementation of julia that follows the spec? Which does make sense 👍 |
|
It would likely fail currently under the interpreter and/or runtime implementations of this, so it is not an ideal test. |
|
Thanks all. One more question: should this be backported to 1.9? Since it's a bugfix? (It's also a perf fix for |
Backported PRs: - [x] #47782 <!-- Generalize Bool parse method to AbstractString --> - [x] #48634 <!-- Remove unused "deps" mechanism in internal sorting keywords [NFC] --> - [x] #49931 <!-- Lock finalizers' lists at exit --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50635 <!-- `versioninfo()`: include build info and unofficial warning --> - [x] #49915 <!-- Revert "Remove number / vector (#44358)" --> - [x] #50781 <!-- fix `bit_map!` with aliasing --> - [x] #50845 <!-- fix #50438, use default pool for at-threads --> - [x] #49031 <!-- Update inference.md --> - [x] #50289 <!-- Initialize prev_nold and nold in gc_reset_page --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #49582 <!-- Update HISTORY.md for `DelimitedFiles` --> - [x] #50341 <!-- invokelatest docs should say not exported before 1.9 --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50523 <!-- Avoid generic call in most cases for getproperty --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50164 <!-- codegen: handle dead code with unsafe_store of FCA pointers --> - [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception handling. --> Need manual backport: - [ ] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [ ] #50591 <!-- build: fix various makefile bugs --> Non-merged PRs with backport label: - [ ] #50842 <!-- Avoid race conditions with recursive rm --> - [ ] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #49716 <!-- Update varinfo() docstring signature --> - [ ] #49713 <!-- prevent REPL from erroring in numbered mode in some situations --> - [ ] #49573 <!-- Implement jl_cpu_pause on PPC64 --> - [ ] #48726 <!-- fix macro expansion of property destructuring --> - [ ] #48642 <!-- Use gc alloc instead of alloc typed in lowering --> - [ ] #48183 <!-- Don't use pkgimage for package if any includes fall in tracked path for coverage or alloc tracking --> - [ ] #48050 <!-- improve `--heap-size-hint` arg handling --> - [ ] #47615 <!-- Allow threadsafe access to buffer of type inference profiling trees -->
Fix #50125