-
-
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
Switch LLVM codegen of Ptr{T} to an actual pointer type. #53687
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
|
We do jl_unwrap_unionall in plenty of places so it seems to be fine i.e emitting builtin_memoryref |
Those uses do not involve unboxing or creating boxes; doing so with the unwrapped type seems to create corrupt objects (segfaulting and aborting all over the place). |
The package evaluation job you requested has completed - possible new issues were detected. |
FWIW, I suspect most of the improvements on the time benchmarks seem to be noise due to a lot of the benchmarks on master being unusually slow. The regressions similarly seem to be on the order of ns or on historically noisy benchmarks. I think it's a bit easier to see here. |
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). |
This PR switches our code generation for
Ptr{T}
fromi64
to an actual LLVM pointer type (ptr
when using opaque pointers, an untypedi8*
otherwise). The main motivation is to simplifyllvmcall
usage (doing away with theinttoptr
/ptrtoint
conversions), and also make it possible to simply useccall
to call intrinsics with pointer arguments (where we currently always needllvmcall
for converting to a pointer).Changing codegen like this is a breaking change for
llvmcall
users, but we don't promise any stability there. Also, with the switch to LLVM 17 where typed pointers have been removed, allllvmcall
snippets 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
ccall
converts 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
type
argument to theGCAllocBytes
intrinsic is currently still aT_size
.