Add alignment to constant globals#50738
Conversation
src/codegen.cpp
Outdated
| Value *loc; | ||
| if (valid_as_globalinit(v)) { // llvm can't handle all the things that could be inside a ConstantExpr | ||
| loc = get_pointer_to_constant(ctx.emission_context, cast<Constant>(v), "_j_const", *jl_Module); | ||
| loc = get_pointer_to_constant(ctx.emission_context, cast<Constant>(v), Align(jl_is_datatype(typ) && jl_struct_try_layout((jl_datatype_t*)typ) ? julia_alignment(typ) : JL_HEAP_ALIGNMENT), "_j_const", *jl_Module); |
There was a problem hiding this comment.
| loc = get_pointer_to_constant(ctx.emission_context, cast<Constant>(v), Align(jl_is_datatype(typ) && jl_struct_try_layout((jl_datatype_t*)typ) ? julia_alignment(typ) : JL_HEAP_ALIGNMENT), "_j_const", *jl_Module); | |
| assert(jl_is_concrete_type(typ)); // not legal to have an unboxed abstract type | |
| loc = get_pointer_to_constant(ctx.emission_context, cast<Constant>(v), Align(julia_alignment(typ)), "_j_const", *jl_Module); |
There was a problem hiding this comment.
This assertion isn't holding, is it actually valid?
There was a problem hiding this comment.
The assert is right, but the callee now is not right anymore. You need to switch to using the copy-constructor, so the type is correct (the tindex argument to value_to_pointer is no longer legal as a means of folding this together):
diff --git a/src/codegen.cpp b/src/codegen.cpp
index 5b0bf1bc98..11fa7474ba 100644
--- a/src/codegen.cpp
+++ b/src/codegen.cpp
@@ -2317,7 +2317,7 @@ static jl_cgval_t convert_julia_type(jl_codectx_t &ctx, const jl_cgval_t &v, jl_
new_tindex = ConstantInt::get(getInt8Ty(ctx.builder.getContext()), new_idx);
if (v.V && !v.ispointer()) {
// TODO: remove this branch once all consumers of v.TIndex understand how to handle a non-ispointer value
- return value_to_pointer(ctx, v.V, typ, new_tindex);
+ return jl_cgval_t(value_to_pointer(ctx, v.V, v.typ), typ, new_tindex);
}
}
else if (jl_subtype(v.typ, typ)) {
(fwiw, the TODO there is about places like this needing to be fixed to use the max-align from jl_islayout_inline instead when deciding how to spill it)
|
There is a couple places it looks like you mixed up values and pointers-to-values, but otherwise SGTM. |
Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
src/codegen.cpp
Outdated
| if (v.V && !v.ispointer()) { | ||
| // TODO: remove this branch once all consumers of v.TIndex understand how to handle a non-ispointer value | ||
| return value_to_pointer(ctx, v.V, typ, new_tindex); | ||
| return jl_cgval_t(value_to_pointer(ctx, v.V, v.typ), typ, new_tindex); |
There was a problem hiding this comment.
ah, it wants the whole thing. But also, you should delete the unused parameter to value_to_pointer now also
| return jl_cgval_t(value_to_pointer(ctx, v.V, v.typ), typ, new_tindex); | |
| return jl_cgval_t(value_to_pointer(ctx, v), typ, new_tindex); |
|
Marking for backport because it technically should affect 1.10 too. |
(cherry picked from commit 3708229)
Backported PRs: - [x] #50637 <!-- Remove SparseArrays legacy code --> - [x] #50665 <!-- print `@time` msg into print buffer --> - [x] #50523 <!-- Avoid generic call in most cases for getproperty --> - [x] #50635 <!-- `versioninfo()`: include build info and unofficial warning --> - [x] #50670 <!-- Make reinterpret specialize fully. --> - [x] #50666 <!-- include `--pkgimage=no` caches for stdlibs --> - [x] #50765 - [x] #50764 - [x] #50768 - [x] #50767 - [x] #50618 <!-- inference: continue const-prop' when concrete-eval returns non-inlineable --> - [x] #50689 <!-- Attach `tanpi` docstring to method --> - [x] #50671 <!-- Fix rdiv of complex lhs by real factorizations --> - [x] #50598 <!-- only limit types in stack traces in the REPL --> - [x] #50766 <!-- Don't partition alwaysinline functions --> - [x] #50771 <!-- re-allow non-string values in ENV `get!` --> - [x] #50682 <!-- Add fallback if we have make a weird GC decision. --> - [x] #50781 <!-- fix `bit_map!` with aliasing --> - [x] #50172 <!-- print feature flags used for matching pkgimage --> - [x] #50844 <!-- Bump OpenBLAS binaries to use the new GEMM multithreading threshold --> - [x] #50826 <!-- Update dependency builds --> - [x] #50845 <!-- fix #50438, use default pool for at-threads --> - [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [x] #50655 <!-- fix hashing regression. --> - [x] #50779 <!-- Minor refactor to image generation --> - [x] #50791 <!-- Make symbols internal in jl_create_native, and only externalize them when partitioning --> - [x] #50724 <!-- Merge opaque closure modules with the rest of the workqueue --> - [x] #50738 <!-- Add alignment to constant globals --> - [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception handling. --> Need manual backport: Contains multiple commits, manual intervention needed: Non-merged PRs with backport label: - [ ] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [ ] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [ ] #50809 <!-- Limit type-printing in MethodError --> - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #50594 <!-- Disallow non-index Integer types in isassigned --> - [ ] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
Should fix #50698