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.

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

Automatically skip boundschecks in setindex! when safe to do so #68

Merged
merged 2 commits into from
Oct 13, 2024

Conversation

giordano
Copy link
Collaborator

@giordano giordano commented Oct 7, 2024

Fix #65

@giordano giordano requested a review from nsajko October 7, 2024 23:08
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.66%. Comparing base (f62e401) to head (c387663).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
+ Coverage   98.64%   98.66%   +0.01%     
==========================================
  Files           1        1              
  Lines         222      225       +3     
==========================================
+ Hits          219      222       +3     
  Misses          3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@giordano
Copy link
Collaborator Author

giordano commented Oct 8, 2024

I'm now away from keyboard, will need to double check this at another time, but if I remember correctly this PR only fixes the issue with bounds checks. Without an explicit @inbounds in front of the setindex! in the for loop the stores aren't vectorised, for some reason that I couldn't understand. Using @inbounds recovers full vectorisation, but that's not needed for Vectors. Maybe an issue for a different ticket.

Edit: I can confirm this doesn't vectorise the stores, but at least removes the bounds checks, which is the main problem reported in #65

julia> code_llvm((FixedSizeVector{Float64,Memory{Float64}},)) do v
           for idx in eachindex(v)
               v[idx] = idx
           end
       end
LLVM IR
; Function Signature: var"#23"(FixedSizeArrays.FixedSizeArray{Float64, 1, Memory{Float64}})
;  @ REPL[16]:2 within `#23`
define void @"julia_#23_3098"(ptr nocapture noundef nonnull readonly align 8 dereferenceable(16) %"v::FixedSizeArray", ptr nocapture readonly %.roots.v) #0 {
top:
; ┌ @ abstractarray.jl:321 within `eachindex`
; │┌ @ abstractarray.jl:137 within `axes1`
; ││┌ @ abstractarray.jl:98 within `axes`
; │││┌ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:64 within `size`
; ││││┌ @ Base.jl:49 within `getproperty`
       %0 = getelementptr inbounds i8, ptr %"v::FixedSizeArray", i64 8
; └└└└└
; ┌ @ range.jl:911 within `iterate`
; │┌ @ range.jl:688 within `isempty`
; ││┌ @ operators.jl:425 within `>`
; │││┌ @ int.jl:83 within `<`
      %.unbox = load i64, ptr %0, align 8
      %1 = icmp slt i64 %.unbox, 1
; └└└└
  br i1 %1, label %L48, label %L13.preheader16

L13.preheader16:                                  ; preds = %top
  %memoryref_mem = load ptr, ptr %.roots.v, align 8
  %memory_data_ptr = getelementptr inbounds { i64, ptr }, ptr %memoryref_mem, i64 0, i32 1
;  @ REPL[16]:3 within `#23`
; ┌ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:240
; │┌ @ boot.jl:544 within `memoryref`
    %memoryref_data.pre = load ptr, ptr %memory_data_ptr, align 8
; │└
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:59 within `setindex!`
   br label %L29

L29:                                              ; preds = %L29, %L13.preheader16
   %value_phi3 = phi i64 [ %4, %L29 ], [ 1, %L13.preheader16 ]
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:239
; │┌ @ number.jl:7 within `convert`
; ││┌ @ float.jl:239 within `Float64`
     %2 = sitofp i64 %value_phi3 to double
; │└└
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:240
   %memoryref_offset = shl i64 %value_phi3, 3
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:241
   %3 = getelementptr i8, ptr %memoryref_data.pre, i64 %memoryref_offset
   %memoryref_data6 = getelementptr i8, ptr %3, i64 -8
   store double %2, ptr %memoryref_data6, align 8
; └
;  @ REPL[16]:4 within `#23`
; ┌ @ range.jl:915 within `iterate`
   %4 = add nuw i64 %value_phi3, 1
; └
  %5 = icmp ult i64 %value_phi3, %.unbox
  br i1 %5, label %L29, label %L48

L48:                                              ; preds = %L29, %top
  ret void
}

Compare with

julia> code_llvm((FixedSizeVector{Float64,Memory{Float64}},)) do v
           for idx in eachindex(v)
               @inbounds v[idx] = idx
           end
       end
LLVM IR
; Function Signature: var"#26"(FixedSizeArrays.FixedSizeArray{Float64, 1, Memory{Float64}})
;  @ REPL[17]:2 within `#26`
define void @"julia_#26_3146"(ptr nocapture noundef nonnull readonly align 8 dereferenceable(16) %"v::FixedSizeArray", ptr nocapture readonly %.roots.v) #0 {
top:
; ┌ @ abstractarray.jl:321 within `eachindex`
; │┌ @ abstractarray.jl:137 within `axes1`
; ││┌ @ abstractarray.jl:98 within `axes`
; │││┌ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:64 within `size`
; ││││┌ @ Base.jl:49 within `getproperty`
       %0 = getelementptr inbounds i8, ptr %"v::FixedSizeArray", i64 8
; └└└└└
; ┌ @ range.jl:911 within `iterate`
; │┌ @ range.jl:688 within `isempty`
; ││┌ @ operators.jl:425 within `>`
; │││┌ @ int.jl:83 within `<`
      %.unbox = load i64, ptr %0, align 8
      %1 = icmp slt i64 %.unbox, 1
; └└└└
  br i1 %1, label %L48, label %L13.preheader

L13.preheader:                                    ; preds = %top
  %memoryref_mem = load ptr, ptr %.roots.v, align 8
  %memory_data_ptr = getelementptr inbounds { i64, ptr }, ptr %memoryref_mem, i64 0, i32 1
  %memoryref_data = load ptr, ptr %memory_data_ptr, align 8
;  @ REPL[17]:4 within `#26`
  %invariant.gep = getelementptr i8, ptr %memoryref_data, i64 -8
  %min.iters.check = icmp ult i64 %.unbox, 8
  br i1 %min.iters.check, label %scalar.ph, label %vector.ph

vector.ph:                                        ; preds = %L13.preheader
  %n.vec = and i64 %.unbox, 9223372036854775800
  %ind.end = or disjoint i64 %n.vec, 1
  br label %vector.body

vector.body:                                      ; preds = %vector.body, %vector.ph
  %index = phi i64 [ 0, %vector.ph ], [ %index.next, %vector.body ]
  %vec.ind = phi <2 x i64> [ <i64 1, i64 2>, %vector.ph ], [ %vec.ind.next, %vector.body ]
  %step.add = add <2 x i64> %vec.ind, <i64 2, i64 2>
  %step.add13 = add <2 x i64> %vec.ind, <i64 4, i64 4>
  %step.add14 = add <2 x i64> %vec.ind, <i64 6, i64 6>
;  @ REPL[17]:3 within `#26`
; ┌ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:239
; │┌ @ number.jl:7 within `convert`
; ││┌ @ float.jl:239 within `Float64`
     %2 = sitofp <2 x i64> %vec.ind to <2 x double>
     %3 = sitofp <2 x i64> %step.add to <2 x double>
     %4 = sitofp <2 x i64> %step.add13 to <2 x double>
     %5 = sitofp <2 x i64> %step.add14 to <2 x double>
; │└└
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:240
   %offset.idx = shl i64 %index, 3
   %6 = or disjoint i64 %offset.idx, 8
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:241
   %7 = getelementptr i8, ptr %invariant.gep, i64 %6
   %8 = getelementptr double, ptr %7, i64 2
   %9 = getelementptr double, ptr %7, i64 4
   %10 = getelementptr double, ptr %7, i64 6
   store <2 x double> %2, ptr %7, align 8
   store <2 x double> %3, ptr %8, align 8
   store <2 x double> %4, ptr %9, align 8
   store <2 x double> %5, ptr %10, align 8
   %index.next = add nuw i64 %index, 8
   %vec.ind.next = add <2 x i64> %vec.ind, <i64 8, i64 8>
   %11 = icmp eq i64 %index.next, %n.vec
   br i1 %11, label %middle.block, label %vector.body

middle.block:                                     ; preds = %vector.body
; └
;  @ REPL[17]:4 within `#26`
  %cmp.n = icmp eq i64 %.unbox, %n.vec
  br i1 %cmp.n, label %L48, label %scalar.ph

scalar.ph:                                        ; preds = %middle.block, %L13.preheader
  %bc.resume.val = phi i64 [ %ind.end, %middle.block ], [ 1, %L13.preheader ]
  br label %L13

L13:                                              ; preds = %L13, %scalar.ph
  %value_phi3 = phi i64 [ %13, %L13 ], [ %bc.resume.val, %scalar.ph ]
;  @ REPL[17]:3 within `#26`
; ┌ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:239
; │┌ @ number.jl:7 within `convert`
; ││┌ @ float.jl:239 within `Float64`
     %12 = sitofp i64 %value_phi3 to double
; │└└
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:240
   %memoryref_offset = shl i64 %value_phi3, 3
; │ @ /Users/mose/.julia/dev/FixedSizeArrays/src/FixedSizeArrays.jl:60 within `setindex!` @ genericmemory.jl:241
   %gep = getelementptr i8, ptr %invariant.gep, i64 %memoryref_offset
   store double %12, ptr %gep, align 8
; └
;  @ REPL[17]:4 within `#26`
; ┌ @ range.jl:915 within `iterate`
; │┌ @ promotion.jl:639 within `==`
    %.not.not = icmp eq i64 %value_phi3, %.unbox
; │└
   %13 = add nuw i64 %value_phi3, 1
; └
  br i1 %.not.not, label %L48, label %L13

L48:                                              ; preds = %L13, %middle.block, %top
  ret void
}

src/FixedSizeArrays.jl Outdated Show resolved Hide resolved
Co-authored-by: Neven Sajko <s@purelymail.com>
@giordano giordano merged commit 7d9f5d5 into main Oct 13, 2024
9 checks passed
@giordano giordano deleted the mg/setindex-boundschecks branch October 13, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iterating over eachindex doesn't remove out-of-bounds error
2 participants