-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Switch LLVM codegen of Ptr{T} to an actual pointer type. #53687
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
Conversation
| if (isa<Instruction>(vx) && !vx->hasName()) | ||
| // CreatePtrToInt may undo an IntToPtr | ||
| setName(ctx.emission_context, vx, "bitcast_coercion"); |
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.
Apparently the LLVM IRBuilder is smart enough to collapse ptrtoint+inttoptr etc into a reference to the original value. If that happens to be an argument, our setName aborts. Given that we can't predict whether that would happen, shouldn't we have setName bail out for non-Instructions/Constants, instead of aborting and having to check at the call site? We now have many of those already:
cgutils.cpp: if (isa<Instruction>(src) && !src->hasName())
cgutils.cpp: if (isa<Instruction>(dst) && !dst->hasName())
intrinsics.cpp: if (isa<Instruction>(vx) && !vx->hasName())
intrinsics.cpp: if (isa<Instruction>(vx) && !vx->hasName())
intrinsics.cpp: if (isa<Instruction>(thePtr) && !thePtr->hasName())
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Very nice! If I'm not mistaken, this should also make things that use |
6f27310 to
ac4227b
Compare
src/intrinsics.cpp
Outdated
| } | ||
| case sub_ptr: { | ||
| assert(nargs == 2); | ||
| if (!jl_is_cpointer_type(argv[0].typ) || argv[1].typ != (jl_value_t*)jl_ulong_type) |
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.
This is adding constraints that weren't previously there, so this is potentially breaking (and since it moved from the generic emit_untyped_intrinsic cases to here, it will also need a custom tfunc)
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.
Yeah, this was on purpose, because currently the emit_untyped_intrinsic logic assumes that the types of the inputs are identical. Since I couldn't find any uses with non-UInt inputs in any public code, it seemed easier to move the pointer intrinsics out of the arithmetic ones (especially wrt. implementing a runtime version; the macros in there are a bit hairy).
I'll mark it as technically breaking though and run PkgEval to make sure nothing breaks. Unless you think it's better to keep the previous, untyped behavior?
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.
Sounds like a good change to me
|
Could this affect pointer provenance/alias analysis? Since ptrtoint inttoptr sometimes wash those away? |
|
I can't reproduce these llvmpasses failures... EDIT: could reproduce them using the CI rootfs images. |
|
@nanosoldier |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Turns out that even on LLVM 17 from #53070, the IR autoupgrader knows how to handle typed pointers: ... so it's probably too breaking to make |
|
Does LLVM have some guarantee that this will continue working in the future? |
|
Not that I know. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Bunch of packages failed to test due to what looks like a Nanosoldier-related issue. I've restarted the machine, so let's try again: @nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
Should the dep warnings be a bit limited? Right now, some packages write endless number of to the logs. |
|
I guess, but we don't have much Another remaining issue is a failed assertion with the following code (reduced from the foobar(ptr) = ptr + UInt(0)
code_llvm(foobar, Tuple{Ptr{T}} where {T})This generates simply: The call to Lines 6180 to 6201 in 9df47f2
The result of Line 1882 in 9df47f2
@vtjnash What's the intended design here; should FWIW, it didn't use to be a problem as everything was EDIT: calling |
|
And PkgEval looks good too: ExaTron failure is a AMDGPU.jl bug (filed downstream), Atomix.jl fails due to overly strict doctests stumbling over the depwarn, and the other failures are unrelated. Rebased, will merge once CI is "green" (with the same failures as currently on master). |
Previously, add_ptr and sub_ptr intrinsics were incorrectly inferred as potentially throwing because they fell through to the general primitive type check, which was incorrect after #53687 changed them. This adds explicit handling in intrinsic_exct to return Union{} (nothrow) when given correct argument types (Ptr and UInt), similar to #57398. Fixes #57557
Previously, add_ptr and sub_ptr intrinsics were incorrectly inferred as potentially throwing because they fell through to the general primitive type check, which was incorrect after #53687 changed them. This adds explicit handling in intrinsic_exct to return Union{} (nothrow) when given correct argument types (Ptr and UInt), similar to #57398. Fixes #57557 Written by Claude
Previously, add_ptr and sub_ptr intrinsics were incorrectly inferred as potentially throwing because they fell through to the general primitive type check, which was incorrect after #53687 changed them. This adds explicit handling in intrinsic_exct to return Union{} (nothrow) when given correct argument types (Ptr and UInt), similar to #57398. Fixes #57557 Written by Claude
Previously, add_ptr and sub_ptr intrinsics were incorrectly inferred as potentially throwing because they fell through to the general primitive type check, which was incorrect after #53687 changed them. This adds explicit handling in intrinsic_exct to return Union{} (nothrow) when given correct argument types (Ptr and UInt), similar to #57398. Fixes #57557 Written by Claude
This PR switches our code generation for
Ptr{T}fromi64to an actual LLVM pointer type (ptrwhen using opaque pointers, an untypedi8*otherwise). The main motivation is to simplifyllvmcallusage (doing away with theinttoptr/ptrtointconversions), and also make it possible to simply useccallto call intrinsics with pointer arguments (where we currently always needllvmcallfor converting to a pointer).Changing codegen like this is a breaking change for
llvmcallusers, but we don't promise any stability there. Also, with the switch to LLVM 17 where typed pointers have been removed, allllvmcallsnippets will have to be updated anyway, so this seems like a good time to make that change.Before:
After:
This also simplifies "real code", e.g., when
ccallconverts an Array to a pointer. I don't expect that to affect performance though.There are a couple of other patterns that could be updated, e.g. the
typeargument to theGCAllocBytesintrinsic is currently still aT_size.