-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 CyclePadding(::DataType)
#50719
fix CyclePadding(::DataType)
#50719
Conversation
@@ -720,7 +720,9 @@ function CyclePadding(T::DataType) | |||
a, s = datatype_alignment(T), sizeof(T) | |||
as = s + (a - (s % a)) % a | |||
pad = padding(T) | |||
s != as && push!(pad, Padding(s, as - s)) | |||
if s != as | |||
pad = Core.svec(pad..., Padding(s, as - s)) |
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.
Is this function still inferable? It seems like as - s
might need to be an argument to padding
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 it's still inferrable:
julia> only(code_typed(Base.CyclePadding, (DataType,); optimize=false))
CodeInfo(
1 ─ %1 = Base.datatype_alignment(T)::Int64
│ %2 = Base.sizeof(T)::Int64
│ (a = %1)::Int64
│ (s = %2)::Int64
│ %5 = s::Int64
│ %6 = a::Int64
│ %7 = (s % a)::Int64
│ %8 = (%6 - %7)::Int64
│ %9 = (%8 % a)::Int64
│ (as = %5 + %9)::Int64
│ (pad = Base.padding(T))::Core.SimpleVector
│ %12 = (s != as)::Bool
└── goto #3 if not %12
2 ─ %14 = Core.svec::Core.Const(Core.svec)
│ %15 = pad::Core.SimpleVector
│ %16 = s::Int64
│ %17 = (as - s)::Int64
│ %18 = Base.Padding(%16, %17)::Base.Padding
│ %19 = Core.tuple(%18)::Tuple{Base.Padding}
└── (pad = Core._apply_iterate(Base.iterate, %14, %15, %19))::Core.SimpleVector
3 ┄ %21 = Base.CyclePadding(pad, as)::Base.CyclePadding{Core.SimpleVector}
└── return %21
) => Base.CyclePadding{Core.SimpleVector}
julia> code_typed(; optimize=false) do
Base.CyclePadding(Int)
end
1-element Vector{Any}:
CodeInfo(
1 ─ %1 = Base.CyclePadding::Core.Const(Base.CyclePadding)
│ %2 = (%1)(Main.Int)::Core.Const(Base.CyclePadding{Core.SimpleVector}(svec(), 8))
└── return %2
) => Base.CyclePadding{Core.SimpleVector}
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 code isn't reachable for as == s
, but generally seems pretty hard to trigger anyways? I don't fully understand this code's (expected/observed) behavior:
julia> code_typed(; optimize=false) do
Base.CyclePadding(Tuple{Int64,Int8,Int16})
end
1-element Vector{Any}:
CodeInfo(
@ REPL[12]:2 within `#19`
1 ─ %1 = Base.CyclePadding::Core.Const(Base.CyclePadding)
│ %2 = Core.apply_type(Main.Tuple, Main.Int64, Main.Int8, Main.Int16)::Core.Const(Tuple{Int64, Int8, Int16})
│ %3 = (%1)(%2)::Core.Const(Base.CyclePadding{Core.SimpleVector}(svec(Base.Padding(10, 1), Base.Padding(16, 4)), 16))
└── return %3
) => Base.CyclePadding{Core.SimpleVector}
julia> primitive type Int24 24 end
julia> Base.CyclePadding(Int24)
ERROR: MethodError: no method matching push!(::Core.SimpleVector, ::Base.Padding)
Closest candidates are:
push!(::Any, ::Any, ::Any)
@ Base abstractarray.jl:3399
push!(::Any, ::Any, ::Any, ::Any...)
@ Base abstractarray.jl:3400
push!(::BitVector, ::Any)
@ Base bitarray.jl:752
...
Stacktrace:
[1] Base.CyclePadding(T::DataType)
@ Base ./reinterpretarray.jl:723
[2] top-level scope
@ REPL[11]:1
Sorry for the noise, but I just saw that this fix was not included in the 1.10.0-beta2. Any chance it has been forgotten somehow, or is it just lagging behind a bit? |
Backported PRs: - [x] #48625 <!-- add replace(io, str, patterns...) --> - [x] #48387 <!-- Improve documentation of sort-related functions --> - [x] #48363 <!-- Revise sort.md and docstrings in sort.jl --> - [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 --> - [x] #50719 <!-- fix `CyclePadding(::DataType)` --> - [x] #50694 <!-- inference: permit recursive type traits --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50594 <!-- Disallow non-index Integer types in isassigned --> - [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid allocation on poptask --> - [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor --> - [x] #50874 <!-- Restrict COFF to a single thread when symbol count is high --> - [x] #50822 <!-- Add default method for setmodifiers! --> - [x] #50730 <!-- Fix integer overflow in `isapprox` --> - [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [x] #50809 <!-- Limit type-printing in MethodError --> - [x] #50915 <!-- Add note the `Task` about sticky bit --> - [x] #50929 <!-- when widening tuple types in tmerge, only widen the complex parts --> - [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 --> - [x] #50959 <!-- Update libssh2 patches --> - [x] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [x] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [x] #50912 <!-- Separate foreign threads into a :foreign threadpool --> - [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD --> - [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse inference --> - [x] #51027 <!-- Implement realloc accounting correctly --> - [x] #51019 <!-- fix a case of potentially use of undefined variable when handling error in distributed message processing --> - [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool}) (#42202)" --> - [x] #51036 <!-- add missing invoke edge for nospecialize targets --> - [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete functions --> - [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 --> - [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration --> - [x] #51154 <!-- broadcast: use recursion rather than ntuple to map over a tuple --> - [x] #51153 <!-- fix debug typo in "add missing invoke edge for nospecialize targets (#51036)" --> - [x] #51222 <!-- Check again if the tty is open inside the IO lock --> - [x] #51236 <!-- Add lock around uv_unref during init --> - [x] #51243 <!-- GMP: Gracefully handle more overflows. --> - [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite trailing dot --> - [x] #51175 <!-- shorten stale_age for cachefile lock --> - [x] #51300 <!-- fix method definition error for bad vararg --> - [x] #51307 <!-- fix force-throw ctrl-C on Windows --> - [x] #51303 <!-- ensure revising structs is safe --> - [x] #51393 - [x] #51403 Need manual backport: - [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols --> - [x] #51053 <!-- Bump Statistics stdlib --> - [x] #51013 <!-- fix #50709, type bound error due to free typevar in sparam env --> - [x] #51305 <!-- reduce test time for rounding and floatfuncs --> Contains multiple commits, manual intervention needed: - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> - [x] #51247 <!-- Check if malloc has succeeded before incrementing gc stats --> - [x] #51294 <!-- use LibGit2_jll for LibGit2 library --> Non-merged PRs with backport label: - [ ] #51132 <!-- Handle `AbstractQ` in concatenation --> - [x] #51029 <!-- testdefs: make sure that if a test set changes the active project, they change it back when they're done --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions --> - [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
We probably want to add a test for this code path.