LLVM assertion failure during StrideArraysCore.jl since Memory{T} change

[maleadt]

As seen on PkgEval:

julia: /workspace/srcdir/llvm-project/llvm/lib/IR/Instructions.cpp:2561: void llvm::InsertValueInst::init(llvm::Value*, llvm::Value*, llvm::ArrayRef<unsigned int>, const llvm::Twine&): Assertion `ExtractValueInst::getIndexedType(Agg->getType(), Idxs) == Val->getType() && "Inserted value must match indexed type!"' failed.

[2047348] signal (6.-6): Aborted
in expression starting at /home/maleadt/.julia/packages/StrideArraysCore/VyBzA/test/runtests.jl:75
pthread_kill at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
raise at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7ffbf8c0571a)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
_ZN4llvm15InsertValueInst4initEPNS_5ValueES2_NS_8ArrayRefIjEERKNS_5TwineE at /tmp/jl_FwegDd/bin/../lib/julia/libLLVM-15jl.so (unknown line)
InsertValueInst at /source/usr/include/llvm/IR/Instructions.h:2640 [inlined]
Create at /source/usr/include/llvm/IR/Instructions.h:2565 [inlined]
CreateInsertValue at /source/usr/include/llvm/IR/IRBuilder.h:2343
emit_new_struct at /source/src/cgutils.cpp:3845
emit_new_struct at /source/src/julia.h:1684
emit_expr at /source/src/codegen.cpp:5729
emit_ssaval_assign at /source/src/codegen.cpp:5178
emit_stmtpos at /source/src/codegen.cpp:5428 [inlined]
emit_function at /source/src/codegen.cpp:8545
jl_emit_code at /source/src/codegen.cpp:8880
jl_emit_codeinst at /source/src/codegen.cpp:8963

Bisected to #51319 (cc @vtjnash @oscardssmith)

[vtjnash]

It is a StrideArraysCore bug. It is using @generated to emit manual Expr(:new) to implement a generic copy for objects, but that is semantically invalid and can result in making malformed IR

[maleadt]

There's another package that triggers this: MCPhylo.jl

julia: /workspace/srcdir/llvm-project/llvm/lib/IR/Instructions.cpp:2561: void llvm::InsertValueInst::init(llvm::Value*, llvm::Value*, llvm::ArrayRef<unsigned int>, const llvm::Twine&): Assertion `ExtractValueInst::getIndexedType(Agg->getType(), Idxs) == Val->getType() && "Inserted value must match indexed type!"' failed.

[177] signal 6 (-6): Aborted
in expression starting at /home/pkgeval/.julia/packages/MCPhylo/KWPlY/test/distributions/phylodist.jl:1
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7fbeac0f040e)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
_ZN4llvm15InsertValueInst4initEPNS_5ValueES2_NS_8ArrayRefIjEERKNS_5TwineE at /opt/julia/bin/../lib/julia/libLLVM-15jl.so (unknown line)
InsertValueInst at /source/usr/include/llvm/IR/Instructions.h:2640 [inlined]
Create at /source/usr/include/llvm/IR/Instructions.h:2565 [inlined]
CreateInsertValue at /source/usr/include/llvm/IR/IRBuilder.h:2343
emit_new_struct at /source/src/cgutils.cpp:3870
emit_new_struct at /source/src/julia.h:1704
emit_expr at /source/src/codegen.cpp:5945
emit_ssaval_assign at /source/src/codegen.cpp:5367
emit_stmtpos at /source/src/codegen.cpp:5642 [inlined]
emit_function at /source/src/codegen.cpp:8810
jl_emit_code at /source/src/codegen.cpp:9144
jl_emit_codeinst at /source/src/codegen.cpp:9227
_jl_compile_codeinst at /source/src/jitlayers.cpp:220
jl_generate_fptr_impl at /source/src/jitlayers.cpp:525
jl_compile_method_internal at /source/src/gf.c:2509 [inlined]
jl_compile_method_internal at /source/src/gf.c:2397
_jl_invoke at /source/src/gf.c:2912 [inlined]
ijl_apply_generic at /source/src/gf.c:3097
logpdf at /home/pkgeval/.julia/packages/MCPhylo/KWPlY/src/distributions/Phylodist.jl:118

At a quick glance, I don't see any Expr(:new) shenanigans going on in https://github.com/erathorn/MCPhylo.jl/blob/5f74874abed33799871e4eeeb7e08aaf534c9e3d/src/distributions/Phylodist.jl#L107-L122 (but it might be there in any of the deps, of course)

[vtjnash]

The bad code generator is in LoopVectorization.jl here: https://github.com/JuliaSIMD/LoopVectorization.jl/blob/d2f749dbf95c161c803f8733e0b4376c441c3a4a/src/condense_loopset.jl#L40

[maleadt]

It is using @generated to emit manual Expr(:new) to implement a generic copy for objects, but that is semantically invalid and can result in making malformed IR

Could you elaborate what's illegal about manual Expr(:new)? There's quite some packages doing this, https://juliahub.com/ui/Search?q=Expr%28%3Anew&type=code, AFAICT the general pattern is to be copy objects bypassing inner constructor limitations. Is there an alternative?

[oscardssmith]

The thing that's illegal is that these codepaths weren't supposed to run for Arrays because they used to not have any fields. Specifically, in the LoopVectorization case, it was

@generated function reassemble_tuple(::Type{T}, t::Tuple) where {T}
  if Base.issingletontype(T)
    return T.instance
  elseif fieldcount(T) ≡ 0
    call = Expr(:call, getfield, :t, 1)
  elseif T <: DataType
    call = Expr(:call, lv(:gettype), Expr(:call, getfield, :t, 1))
  else
    call, _ = rebuild_fields(0, T)
  end

so Array used to be a non-singleton with 0 fields which meant that it was skipped by this logic. Now that Array has fields, this code ends up recursing down to Memory which is a partially opaque object, so it has fields, but can not be reconstructed from it's fields.

[maleadt]

Thanks!

[vtjnash]

That code probably should not recurse for any immutable object or MemoryRef, since that violates object identity in many cases. For immutable objects, they don't have identify anyways

[ranocha]

Could you maybe suggest how the code should look like? It looks like LoopVectorization.jl was deprecated because of this for Julia 1.11, which is quite sad given the nice performance improvements it can yield.

[vtjnash]

In offline discussion, I am told that part of the code is a benchmarking microoptimization, and can simply be deleted with no adverse effect on the package or users

[brenhinkeller]

I have to wonder if @vtjnash being so curt/rude about everything here added to the deprecation

[KristofferC]

Reading #52702 (comment), it seems the particular fix here is almost trivial so I doubt that the actual change here that caused the assertion has much to do with it.

[brenhinkeller]

It does seem to be a "straw that breaks the camels back" type situation

[KristofferC]

What are the other straws?

[brenhinkeller]

I assume just the significant ongoing maintenance burden as a result of changes to various unstable Julia internals that LV necessarily depends on

[maleadt]

Reading #52702 (comment), it seems the particular fix here is almost trivial

Sadly, that doesn't seem like it's the only required fix. I implemented specifically that change in JuliaSIMD/LoopVectorization.jl#524, but other crashes remain, as documented in JuliaSIMD/LoopVectorization.jl#525 (comment).

[vtjnash]

A lot of that code apparently can be simply deleted, as it is an optimization for some benchmarks which isn't generally correct and slows down the compiler (as well as makes is crash-y)