Skip to content

Avoid recursive store for String #2263

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Avoid recursive store for String #2263

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b2bb25
Avoid recursive store for String
vchuravy Jan 13, 2025
53c0911
iff fieldcount is 0, don't create recursive stores
vchuravy Feb 18, 2025
2c17f4c
use fallback
vchuravy Apr 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ function julia_undef_value_for_type(
end

function create_recursive_stores(B::LLVM.IRBuilder, @nospecialize(Ty::DataType), @nospecialize(prev::LLVM.Value))::Nothing
if Base.datatype_pointerfree(Ty)
if Base.datatype_pointerfree(Ty) || Base.datatype_fieldcount(Ty) == 0
Copy link
Member

Choose a reason for hiding this comment

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

this I think is fine though

return
end

Expand Down Expand Up @@ -647,7 +647,7 @@ function create_recursive_stores(B::LLVM.IRBuilder, @nospecialize(Ty::DataType),
LLVM.Value[LLVM.ConstantInt(Int64(off))],
)

fallback = Base.isabstracttype(Ty2) || Ty2 isa Union
fallback = Base.isabstracttype(Ty2) || Ty2 isa Union || Base.datatype_fieldcount(Ty2) == 0
Copy link
Member

Choose a reason for hiding this comment

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

so I'm not sure this is right. Essentially this is checking whether the inline-data field type can be initialized as a null jlvaluet

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I understand.

This does mean thatfallback = Base.isabstracttype(Ty2) || Ty2 isa Union || Ty2 isa Symbol || Ty2 isa String

Is not correct either, since the field descriptor of the parent wouldn't contain the information that this field may be undefined. E.g. We would need to store a reference to a zero(Ty2) instead? Otherwise, printing the tape will seqfault.

In Julia one can only have unintialized fields at the end of the layout. We statically look at the constructor to determine which fields may be uninitialized.

mutable struct UndefField
           x::String
           UndefField() = new()
end

julia> Core.Compiler.datatype_min_ninitialized(UndefField)
0

mutable struct UndefField2
            y::Symbol
            x::String
            UndefField2() = new(:x)
 end

julia> Core.Compiler.datatype_min_ninitialized(UndefField2)
1

access(obj) = obj.y

define swiftcc nonnull ptr @julia_access_1103(ptr nonnull swiftself %pgcstack, ptr noundef nonnull align 8 dereferenceable(16) %"obj::UndefField2") #0 !dbg !5 {
top:
;  @ REPL[2] within `access`
  call void @llvm.dbg.value(metadata ptr undef, metadata !20, metadata !DIExpression(DW_OP_deref)), !dbg !21
  call void @llvm.dbg.value(metadata ptr undef, metadata !20, metadata !DIExpression(DW_OP_deref)), !dbg !21
  %ptls_field = getelementptr inbounds ptr, ptr %pgcstack, i64 2
  %ptls_load = load ptr, ptr %ptls_field, align 8, !tbaa !22
  %0 = getelementptr inbounds ptr, ptr %ptls_load, i64 2
  %safepoint = load ptr, ptr %0, align 8, !tbaa !26
  fence syncscope("singlethread") seq_cst
;  @ REPL[2]:1 within `access`
  %1 = load volatile i64, ptr %safepoint, align 8, !dbg !28
  fence syncscope("singlethread") seq_cst
; ┌ @ Base.jl:49 within `getproperty`
   %.y = load atomic ptr, ptr %"obj::UndefField2" unordered, align 8, !dbg !29, !tbaa !33, !alias.scope !37, !noalias !40, !nonnull !15
   ret ptr %.y, !dbg !29
; └
}

We mark the return and the load as non-null.

Whereas:

julia> access_x(obj) = obj.x
access_x (generic function with 1 method)

julia> @code_llvm raw=true access_x(obj)
; Function Signature: access_x(Main.UndefField2)
;  @ REPL[7]:1 within `access_x`
define swiftcc nonnull ptr @julia_access_x_1184(ptr nonnull swiftself %pgcstack, ptr noundef nonnull align 8 dereferenceable(16) %"obj::UndefField2") #0 !dbg !5 {
top:
;  @ REPL[7] within `access_x`
  call void @llvm.dbg.value(metadata ptr undef, metadata !20, metadata !DIExpression(DW_OP_deref)), !dbg !21
  call void @llvm.dbg.value(metadata ptr undef, metadata !20, metadata !DIExpression(DW_OP_deref)), !dbg !21
  %ptls_field = getelementptr inbounds ptr, ptr %pgcstack, i64 2
  %ptls_load = load ptr, ptr %ptls_field, align 8, !tbaa !22
  %0 = getelementptr inbounds ptr, ptr %ptls_load, i64 2
  %safepoint = load ptr, ptr %0, align 8, !tbaa !26
  fence syncscope("singlethread") seq_cst
;  @ REPL[7]:1 within `access_x`
  %1 = load volatile i64, ptr %safepoint, align 8, !dbg !28
  fence syncscope("singlethread") seq_cst
; ┌ @ Base.jl:49 within `getproperty`
   %.x_ptr = getelementptr inbounds i8, ptr %"obj::UndefField2", i64 8, !dbg !29
   %.x = load atomic ptr, ptr %.x_ptr unordered, align 8, !dbg !29, !tbaa !33, !alias.scope !37, !noalias !40
   %.not = icmp eq ptr %.x, null, !dbg !29
   br i1 %.not, label %fail, label %pass, !dbg !29

fail:                                             ; preds = %top
   %jl_undefref_exception = load ptr, ptr @jl_undefref_exception, align 8, !dbg !29, !tbaa !26, !alias.scope !45, !noalias !46, !nonnull !15
   call void @ijl_throw(ptr nonnull %jl_undefref_exception), !dbg !29
   unreachable, !dbg !29

pass:                                             ; preds = %top
   ret ptr %.x, !dbg !29
; └
}

So if we may or may not initialize a value as null jl_value_t depends not on the value, but on the layout of the parent.


@static if VERSION < v"1.11-"
fallback |= Ty2 <: Array
Expand Down
Loading