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.

Sign up for GitHub

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

Jump to bottom

Add alignment to constant globals #50738

Merged
merged 5 commits into from
Aug 2, 2023
Merged

Add alignment to constant globals #50738

merged 5 commits into from
Aug 2, 2023

Conversation

pchintalapudi
Copy link
Member

Should fix #50698

@pchintalapudi pchintalapudi added compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM labels Jul 31, 2023
vtjnash
vtjnash reviewed Jul 31, 2023
View reviewed changes
src/cgutils.cpp Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Jul 31, 2023
View reviewed changes
src/cgutils.cpp Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Jul 31, 2023
View reviewed changes
src/codegen.cpp Outdated
@@ -1943,7 +1944,7 @@ static inline jl_cgval_t value_to_pointer(jl_codectx_t &ctx, Value *v, jl_value_
{
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion isn't holding, is it actually valid?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

vtjnash
vtjnash reviewed Jul 31, 2023
View reviewed changes
src/codegen.cpp Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Jul 31, 2023

There is a couple places it looks like you mixed up values and pointers-to-values, but otherwise SGTM.

pchintalapudi
pchintalapudi commented Jul 31, 2023
View reviewed changes
src/cgutils.cpp Outdated Show resolved Hide resolved
@pchintalapudi @vtjnash
Apply suggestions from code review
bef5737 
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
pchintalapudi
pchintalapudi commented Jul 31, 2023
View reviewed changes
src/cgutils.cpp Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jul 31, 2023
@pchintalapudi @vtjnash
Fix value_to_pointer call in convert_julia_type
fda2a31 
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
vtjnash
vtjnash reviewed Aug 1, 2023
View reviewed changes
src/codegen.cpp Outdated
@@ -2317,7 +2319,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, it wants the whole thing. But also, you should delete the unused parameter to value_to_pointer now also

Suggested change
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);

@pchintalapudi pchintalapudi merged commit 3708229 into master Aug 2, 2023
1 check passed
@pchintalapudi pchintalapudi deleted the pc/alignment branch August 2, 2023 00:25
@pchintalapudi pchintalapudi added backport 1.10 Change should be backported to the 1.10 release and removed merge me PR is reviewed. Merge when all tests are passing labels Aug 2, 2023
@pchintalapudi
Copy link
Member Author

Marking for backport because it technically should affect 1.10 too.

@KristofferC KristofferC mentioned this pull request Aug 10, 2023
Merged
36 tasks
pchintalapudi added a commit that referenced this pull request Aug 10, 2023
@pchintalapudi
Add alignment to constant globals (#50738)
6ba470d 
(cherry picked from commit 3708229)
KristofferC added a commit that referenced this pull request Aug 16, 2023
@KristofferC
Backports for julia v1.10.0-beta2 (#50708)
77e7e73 
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 -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
compiler:codegen Generation of LLVM IR and native code compiler:llvm For issues that relate to LLVM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

missing alignment of _j_const in codegen
3 participants
@pchintalapudi @vtjnash @KristofferC