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

Couple ispointer fixes #15312

Merged
merged 1 commit into from
Mar 2, 2016
Merged

Couple ispointer fixes #15312

merged 1 commit into from
Mar 2, 2016

Conversation

carnaval
Copy link
Contributor

@carnaval carnaval commented Mar 1, 2016

  • Do not copy a getfield argument if it is already a pointer.
  • Mark struct-as-pointer arguments immutable

Should: fix #15274, fix #13305, fix #15277

@KristofferC @simonster can you please check that it is all better ?

Also thanks to @vtjnash for the much nicer codegen.cpp now

- Do not copy a getfield argument if it is already a pointer.
- Mark struct-as-pointer arguments immutable

Should: fix #15274, fix #13305, fix #15277
@KristofferC
Copy link
Member

#15274 looks fixed, #15277 looks fixed.

#13305 no longer has the alloc, load, store triplet.

Really nice, thanks for fixing this so quick.

@JeffBezanson
Copy link
Member

💯

@KristofferC
Copy link
Member

The FixedSizeArrays code example from #15274 is also fixed with this.

@tkelman
Copy link
Contributor

tkelman commented Mar 1, 2016

Nice. Is this testable in any more direct way than doing regular perf tracking?

@carnaval
Copy link
Contributor Author

carnaval commented Mar 1, 2016

I'm not sure there is a reasonable way since we don't have first class llvm IR in the language and trying to parse it (or look at the size of the text ...) feels quite brittle

@KristofferC
Copy link
Member

I added a benchmark for this in JuliaCI/BaseBenchmarks.jl#7. If I did it correctly this PR should show a good performance gain for this.

Should be something like runbenchmarks("tuple", vs = "JuliaLang/julia:master"). Since there is only that benchmark with the tuple tag it should go fast.

@KristofferC
Copy link
Member

If someone could run that for me :)

@mbauman
Copy link
Member

mbauman commented Mar 1, 2016

runbenchmarks("tuple", vs = "JuliaLang/julia:master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

@jrevels
Copy link
Member

jrevels commented Mar 2, 2016

I added a benchmark for this in JuliaCI/BaseBenchmarks.jl#7. If I did it correctly this PR should show a good performance gain for this.

Sorry, changes to that repo don't automatically deploy to @nanosoldier - I normally tag a version and do it manually, so that I can do a full test sweep of the CI cycle first. I didn't have time to do it yesterday, but I can deploy the change tomorrow.

@KristofferC
Copy link
Member

Okay, no worries.

carnaval added a commit that referenced this pull request Mar 2, 2016
@carnaval carnaval merged commit cea29d4 into master Mar 2, 2016
@carnaval carnaval deleted the ob/ptrgetf branch March 2, 2016 14:20
// just compute the pointer and let user load it when necessary
Type *fty = julia_type_to_llvm(jt);
Value *addr = builder.CreateGEP(builder.CreatePointerCast(ptr, PointerType::get(fty,0)), idx);
jl_cgval_t fieldval = mark_julia_slot(addr, jt);
Copy link
Member

Choose a reason for hiding this comment

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

fieldval.isimmutable = true?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants