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.

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

Jump to bottom

Migrate to opaque pointers #112

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Migrate to opaque pointers #112

wants to merge 19 commits into from

Conversation

miguelraz
Copy link

This PR

  • Temporarily disables Aqua and some tests to speed up test feedback. These will be turn on again after the corresponding tests pass
  • deletes JULIAPOINTER = "i32"/"i64" and instead uses ptr everywhere

Once the current set of tests pass I'll work on the remaining Special Functions and friends test sets but wanted to get some progress here first.

miguelraz
miguelraz commented May 12, 2024
View reviewed changes
src/llvm_intrin/memory_addr.jl
@@ -314,13 +314,13 @@ function offset_ptr(
if forgep # if forgep, just return now
push!(
instrs,
"%ptr.$(i) = ptrtoint $(index_gep_typ)* %ptr.$(i-1) to $JULIAPOINTERTYPE"
"%ptr.$(i) = load $(vtyp)*, %ptr.$(i-1)"
Copy link
Author

Choose a reason for hiding this comment

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

This one I'm not sure how to bring back the $(vtyp)*

Copy link
Member

@chriselrod chriselrod May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

$(vtyp)* does not exist anymore in an opaque pointer world. It should just be ptr.

Every bitcast from one pointer type to another pointer type should just be deleted without replacement.
Every ptrtoint should just be deleted without replacement.
Every inttoptr should just be deleted without replacement.

Minor fixes will be needed:

%y = bitcast sometype* %x to someothertype*
call foo(%y)

needs to become

call foo(%x)

but that should be relatively trivial when x is ptr.$(i) and y is ptr.$(i+1): just delete the i+=1 along with the bitcast/ptrtoint/intoptr statement.

But that's why I'd also simplify things by having the initial definition (as an argument to the function) be named %ptr.0 instead of %0 as it is currently.

@miguelraz
Copy link
Author

Currently getting a segfault and the trace here is very overwhelming - I've looked at each of these addresses but they don't lead me to places where I've changed the code around.

(running Pkg.test())
masks.jl

[25965] signal 11 (1): Segmentation fault
in expression starting at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/test/runtests.jl:8
macro expansion at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/src/llvm_intrin/memory_addr.jl:1417 [inlined]
__vstore! at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/src/llvm_intrin/memory_addr.jl:1417 [inlined]
macro expansion at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/src/vecunroll/memory.jl:2886 [inlined]
__vstore! at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/src/vecunroll/memory.jl:2886 [inlined]
_vstore! at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/src/strided_pointers/stridedpointers.jl:199 [inlined]
vstore! at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/src/llvm_intrin/memory_addr.jl:2094
unknown function (ip: 0x74c1ec58efdd)
macro expansion at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/test/runtests.jl:412 [inlined]
macro expansion at /cache/build/tester-amdci4-13/julialang/julia-master/usr/share/julia/stdlib/v1.12/Test/src/Test.jl:1700 [inlined]
macro expansion at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/test/runtests.jl:227 [inlined]
macro expansion at ./timing.jl:578 [inlined]
macro expansion at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/test/runtests.jl:224 [inlined]
macro expansion at /cache/build/tester-amdci4-13/julialang/julia-master/usr/share/julia/stdlib/v1.12/Test/src/Test.jl:1700 [inlined]
macro expansion at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/test/runtests.jl:11 [inlined]
macro expansion at ./timing.jl:578 [inlined]
top-level scope at /home/miguelraz/oss/LOOPS/VectorizationBase.jl/test/runtests.jl:312
jl_toplevel_eval_flex at /cache/build/tester-amdci4-13/julialang/julia-master/src/toplevel.c:960
jl_toplevel_eval_flex at /cache/build/tester-amdci4-13/julialang/julia-master/src/toplevel.c:909
ijl_toplevel_eval at /cache/build/tester-amdci4-13/julialang/julia-master/src/toplevel.c:980
ijl_toplevel_eval_in at /cache/build/tester-amdci4-13/julialang/julia-master/src/toplevel.c:1022
eval at ./boot.jl:432 [inlined]

chriselrod
chriselrod reviewed May 12, 2024
View reviewed changes
src/llvm_intrin/memory_addr.jl
@@ -168,7 +168,7 @@ function offset_ptr(
if iszero(O)
push!(
instrs,
"%ptr.$(i) = inttoptr $(JULIAPOINTERTYPE) %0 to $(index_gep_typ)*"
Copy link
Member

Choose a reason for hiding this comment

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

They shouldn't be loads at all. There shouldn't be statements here at all; they should simply be deleted.

I'd suggest renaming the argument from @foo(ptr %0) to @foo(ptr %ptr.0) so that you don't need special handling here for the initial definition.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, this maneuver to get a %ptr.0 is a bit trickier than I though. I reannotated the call sites of @foo(ptr %ptr.0) but am still not finding all the ccases.

@chriselrod
Copy link
Member

Currently getting a segfault and the trace here is very overwhelming - I've looked at each of these addresses but they don't lead me to places where I've changed the code around.

Yeah, segfaults are going to happen when loading and storing to totally invalid pointers.

miguelraz
miguelraz commented May 12, 2024
View reviewed changes
src/llvm_intrin/memory_addr.jl
)
i += 1
i += 1=#
Copy link
Author

Choose a reason for hiding this comment

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

Yes, this comment strategy looks horrible for now and once I'm confident the refactor works I'll delete the lines in a newer commit.

miguelraz
miguelraz commented May 12, 2024
View reviewed changes
src/llvm_intrin/memory_addr.jl
decl = "declare void @llvm.lifetime.end(i64, $ptyp* nocapture)"
instrs = "%ptr = inttoptr $JULIAPOINTERTYPE %0 to $ptyp*\ncall void @llvm.lifetime.end(i64 $L, $ptyp* %ptr)\nret void"
decl = "declare void @llvm.lifetime.end(i64, ptr nocapture)"
instrs = "\ncall void @llvm.lifetime.end(i64 $L, ptr %ptr.0)\nret void"
Copy link
Author

Choose a reason for hiding this comment

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

I know this should be %0 here, but I couldn't figure out how to rename it to %ptr.0.

Copy link
Member

Choose a reason for hiding this comment

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

Name it whatever you're calling it in the function.

miguelraz
miguelraz commented May 12, 2024
View reviewed changes
src/llvm_intrin/memory_addr.jl
push!(
instrs,
"call void @llvm.masked.compressstore.$(suffix(W,T))($vtyp %0, $typ* %ptr, <$W x i1> %mask.0)\nret void"
"call void @llvm.masked.compressstore.$(suffix(W,T))($vtyp %0, ptr %ptr.0, <$W x i1> %mask.0)\nret void"
Copy link
Author

Choose a reason for hiding this comment

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

Same here - didn't know how to get the rename to %ptr -> %ptr.0, or what the full context of the %1 was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriselrod chriselrod chriselrod left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@miguelraz @chriselrod