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

Fix codegen for getfield of homogeneous tuples #34208

Closed
wants to merge 2 commits into from
Closed

Fix codegen for getfield of homogeneous tuples #34208

wants to merge 2 commits into from

Conversation

Keno
Copy link
Member

@Keno Keno commented Dec 28, 2019

This fastpath wasn't checking whether or not the element type
was inline allocated or not.

Fixes #34206
Fixes #34207

This fastpath wasn't checking whether or not the element type
was inline allocated or not.

Fixes #34206
Fixes #34207
@Keno Keno requested review from vtjnash and JeffBezanson December 28, 2019 18:34
@vtjnash
Copy link
Member

vtjnash commented Dec 28, 2019

Much thanks for this! I think this is perhaps just a design issue with typed_load? It's rather awkward that all callers now avoid calling it unless they're fairly confident that they won't accidentally hit the isboxed case. Seems like that should have been an argument to the function instead?

@JeffBezanson
Copy link
Member

Yes, it would be nice if we could preserve this fast path for the case where the element type is known not to be inlinealloc.

Co-Authored-By: Jameson Nash <[email protected]>
@@ -7143,7 +7143,12 @@ Base.iterate(s::SplatBadIterate, args...) = ()

# Issue #34206/34207
function mre34206(a)
b = ntuple(_ -> view(a, :), 1)[1]
function mre34206(a, n)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like something went wrong here?

@JeffBezanson JeffBezanson added this to the 1.4 milestone Jan 2, 2020
@JeffBezanson
Copy link
Member

Should julia_type_to_llvm be checking inlinealloc instead of jl_is_concrete_immutable?

@JeffBezanson
Copy link
Member

How about something like this:

--- a/src/cgutils.cpp
+++ b/src/cgutils.cpp
@@ -1321,6 +1321,10 @@ static jl_cgval_t typed_load(jl_codectx_t &ctx, Value *ptr, Value *idx_0
     Type *elty = julia_type_to_llvm(jltype, &isboxed);
     if (type_is_ghost(elty))
         return ghostValue(jltype);
+    if (!isboxed && !jl_datatype_isinlinealloc(jltype)) {
+        elty = T_prjlvalue;
+        isboxed = true;
+    }
     Type *ptrty = PointerType::get(elty, ptr->getType()->getPointerAddressSpace());

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

Successfully merging this pull request may close these issues.

Unreachable reached Malformed view created by ntuple on master
4 participants