-
-
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
codegen,tbaa: fix array isassigned tbaa information #32356
Conversation
This avoids a regression (correctness and performance) caused by #21262 where we were no longer able to fold away the isnull assertion in the case where the source also contains an explicit isassigned check. Also upgrade many CreateInBoundsGEP calls to include the element type (NFC).
Value *res = ctx.builder.CreateZExt(ctx.builder.CreateICmpNE(load, V_null), T_int32); | ||
Value *arrayptr = emit_bitcast(ctx, emit_arrayptr(ctx, aryv, aryex), T_pprjlvalue); | ||
Value *slot_addr = ctx.builder.CreateInBoundsGEP(T_prjlvalue, arrayptr, idx); | ||
Value *load = tbaa_decorate(tbaa_ptrarraybuf, ctx.builder.CreateLoad(T_prjlvalue, slot_addr)); |
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.
Good catch.
Do you have a simple test case for the correctness regression? Maybe something like checking isassigned, assigning to the array and then checking isassigned again? Because of the incorrect TBAA, it'd probably GVN the two isassigned calls. |
Oddly, it seems to block the desired load-store forwarding optimization (how I noticed the problem), but doesn't trigger a GVN mistake. with: function test(a, i)
@inbounds begin
if !isassigned(a, i)
a[i] = nothing
end
return isassigned(a, i)
end
end
test(Vector(undef, 1), 1) |
This avoids a regression (correctness and performance) caused by #21262 where we were no longer able to fold away the isnull assertion in the case where the source also contains an explicit isassigned check. Also upgrade many other CreateInBoundsGEP calls to include the element type (NFC). (cherry picked from commit a7427aa)
This avoids a regression (correctness and performance) caused by #21262 where we were no longer able to fold away the isnull assertion in the case where the source also contains an explicit isassigned check. Also upgrade many other CreateInBoundsGEP calls to include the element type (NFC). (cherry picked from commit a7427aa)
This avoids a regression (correctness and performance) caused by #21262 where we were no longer able to fold away the isnull assertion in the case where the source also contains an explicit isassigned check. Also upgrade many other CreateInBoundsGEP calls to include the element type (NFC). (cherry picked from commit a7427aa)
This avoids a regression (correctness and performance) caused by #21262 where we were no longer able to fold away the isnull assertion in the case where the source also contains an explicit isassigned check. Also upgrade many other CreateInBoundsGEP calls to include the element type (NFC). (cherry picked from commit a7427aa)
This avoids a regression (correctness and performance) caused by #21262
where we were no longer able to fold away the isnull assertion in the
case where the source also contains an explicit isassigned check.
Also upgrade many CreateInBoundsGEP calls to include the element type (NFC).