Repository navigation
Incorrect result copying array-typed field of structured array in 0.55.0rc1 #7693
Description
Activity
I can reproduce this - unfortunately we've gone from getting a lowering error in earlier versions:
numba.core.errors.LoweringError: Failed in nopython mode pipeline (step: nopython mode backend) Can only insert i16 at [0] in [3 x i16]: got i8* File "repro.py", line 18: def copy(index, src, dest): <source elided> dest['user1'] = src[index]['user'] dest['array1'] = src[index]['array'] ^ During: lowering "dest['array1'] = $26binary_subscr.11" at repro.py (18)to silently doing the wrong thing - I think of this as a regression, and we should have a look at fixing it for 0.55. I think this will probably have been enabled by #7359 (cc @luk-f-a in case he has any thoughts).
I think it was probably enabled by #7359. It's interesting that these work fine
def get(index, src, dest): return src[index]['array'] def set(index, src, dest): dest['array1'][:] = np.ones(3)
and this one throws a typing error
def set(index, src, dest): dest['array1'] = np.ones(3)
BUT
@njit def set2(index, src, dest): dest['array1'] = src[index]['array']
does not throw an error. Probably related to the difference in typing between
arrayandnestedarray.I will have a look today.
small update: looking at the numba ir I didn't spot any obvious problems. there's a
$10binary_subscr.4 = static_getitem(value=$6binary_subscr.2, index=array, index_var=$const8.3, fn=<built-in function getitem>) dest['array1'] = $10binary_subscr.4which looks fine. I'll have to look into what LLVM IR this generates.
small update: looking at the numba ir I didn't spot any obvious problems. there's a
$10binary_subscr.4 = static_getitem(value=$6binary_subscr.2, index=array, index_var=$const8.3, fn=<built-in function getitem>) dest['array1'] = $10binary_subscr.4which looks fine. I'll have to look into what LLVM IR this generates.
Thanks for working on this,
Related issue I found whilst experimenting:
import numpy as np from numba import njit dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True) @njit def ex1(dest): tmp = (np.arange(3) + 1).astype(np.int16) dest['array1'] = tmp dest = np.empty(5, dtype=dest_dtype) ex1(dest[0])
gives:
Traceback (most recent call last): File "/home/gmarkall/numbadev/issues/7693/ex1.py", line 14, in <module> ex1(dest[0]) File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 487, in _compile_for_args raise e File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 420, in _compile_for_args return_val = self.compile(tuple(argtypes)) File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 965, in compile cres = self._compiler.compile(args, return_type) File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 125, in compile status, retval = self._compile_cached(args, return_type) File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 139, in _compile_cached retval = self._compile_core(args, return_type) File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 152, in _compile_core cres = compiler.compile_extra(self.targetdescr.typing_context, File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 693, in compile_extra return pipeline.compile_extra(func) File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 429, in compile_extra return self._compile_bytecode() File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 497, in _compile_bytecode return self._compile_core() File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 472, in _compile_core raise e File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 463, in _compile_core pm.run(self.state) File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 349, in run raise e File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 341, in run self._runPass(idx, pass_inst, state) File "/home/gmarkall/numbadev/numba/numba/core/compiler_lock.py", line 35, in _acquire_compile_lock return func(*args, **kwargs) File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 296, in _runPass mutated |= check(pss.run_pass, internal_state) File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 269, in check mangled = func(compiler_state) File "/home/gmarkall/numbadev/numba/numba/core/typed_passes.py", line 396, in run_pass lower.lower() File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 183, in lower self.lower_normal_function(self.fndesc) File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 237, in lower_normal_function entry_block_tail = self.lower_function_body() File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 266, in lower_function_body self.lower_block(block) File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 280, in lower_block self.lower_inst(inst) File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 483, in lower_inst return impl(self.builder, (target, inst.index, value)) File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 1201, in __call__ res = self._imp(self._context, builder, self._sig, args, loc=loc) File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 1231, in wrapper return fn(*args, **kwargs) File "/home/gmarkall/numbadev/numba/numba/np/arrayobj.py", line 2640, in record_static_setitem_str return impl(builder, (rec, val)) File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 641, in wrapped return impl(self, builder, sig, args, attr) File "/home/gmarkall/numbadev/numba/numba/core/imputils.py", line 166, in res return real_impl(context, builder, sig, args, attr) File "/home/gmarkall/numbadev/numba/numba/np/arrayobj.py", line 2604, in record_setattr val = context.cast(builder, val, valty, elemty) File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 713, in cast return impl(self, builder, fromty, toty, val) File "/home/gmarkall/numbadev/numba/numba/np/arrayobj.py", line 5410, in array_to_array assert fromty.mutable != toty.mutable or toty.layout == 'A' AssertionErrorWith:
diff --git a/numba/np/arrayobj.py b/numba/np/arrayobj.py index a7046a404..ce3a347af 100644 --- a/numba/np/arrayobj.py +++ b/numba/np/arrayobj.py @@ -5407,7 +5407,8 @@ def array_argsort(context, builder, sig, args): @lower_cast(types.Array, types.Array) def array_to_array(context, builder, fromty, toty, val): # Type inference should have prevented illegal array casting. - assert fromty.mutable != toty.mutable or toty.layout == 'A' + layout_compatible = toty.layout == 'A' or fromty.layout == toty.layout + assert fromty.mutable != toty.mutable or layout_compatible return val
(which I think is a reasonable change) the
AssertionErroris no longer hit, but the answer is still not correct:import numpy as np from numba import njit dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True) @njit def ex1(dest): tmp = (np.arange(3) + 1).astype(np.int16) dest['array1'] = tmp dest_nb = np.empty(5, dtype=dest_dtype) dest_py = dest_nb.copy() ex1.py_func(dest_py[0]) ex1(dest_nb[0]) print('Expected:', dest_py[0]) print('Actual: ', dest_nb[0])
gives:
Expected: ([1, 2, 3],) Actual: ([-25280, 16338, 21945],)Modifying the example above so that the prior value of
dest_nbis shown suggests that 4 bytes of garbage is written (as opposed to nothing being written and us seeing some old garbage values)import numpy as np from numba import njit dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True) @njit def ex1(dest): tmp = (np.arange(3) + 1).astype(np.int16) dest['array1'] = tmp dest_nb = np.empty(5, dtype=dest_dtype) dest_py = dest_nb.copy() print('Original: ', dest_nb[0]) ex1.py_func(dest_py[0]) ex1(dest_nb[0]) print('Expected:', dest_py[0]) print('Actual: ', dest_nb[0])
Original: ([-15760, -18323, 22029],) Expected: ([1, 2, 3],) Actual: ([-26096, -18253, 22029],)Also this sometimes segfaults:
Original: ([-32512, 1701, 21847],) Expected: ([1, 2, 3],) Actual: ([11712, 1786, 21847],) Segmentation fault (core dumped)This is handy, as valgrind should help to pinpoint where we're doing some bad memory accesses.
As suggested by @stuartarchibald, somewhere the data of the record is likely to be getting interpreted as an array struct.
Is this what's happening:
; Function Attrs: nofree norecurse nounwind writeonly define i32 @_ZN8__main__7ex1_241B42c8tJTIeFCjyCbUFRqqOAK_2f6h0kCng1maAA_3d_3dE6RecordILi374EE5ArrayIsLi1E1C7mutable7alignedE(i8** noalias nocapture %retptr, { i8*, i32, i8* }** noalias nocapture readnone %excinfo, [6 x i8]* nocapture %arg.dest, i8* %arg.tmp.0, i8* %arg.tmp.1, i64 %arg.tmp.2, i64 %arg.tmp.3, i16* %arg.tmp.4, i64 %arg.tmp.5.0, i64 %arg.tmp.6.0) local_unnamed_addr #0 { entry: ; meminfo %.18.repack = bitcast [6 x i8]* %arg.dest to i8** store i8* %arg.tmp.0, i8** %.18.repack, align 8 ; parent %.18.repack1 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 1, i64 2 %0 = bitcast i8* %.18.repack1 to i8** store i8* %arg.tmp.1, i8** %0, align 8 ; nitems %.18.repack3 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 2, i64 4 %1 = bitcast i8* %.18.repack3 to i64* store i64 %arg.tmp.2, i64* %1, align 8 ; itemsize %.18.repack5 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 4 %2 = bitcast [6 x i8]* %.18.repack5 to i64* store i64 %arg.tmp.3, i64* %2, align 8 ; data %.18.repack7 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 5, i64 2 %3 = bitcast i8* %.18.repack7 to i16** store i16* %arg.tmp.4, i16** %3, align 8 ; shape %.18.repack9 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 6, i64 4 %4 = bitcast i8* %.18.repack9 to i64* store i64 %arg.tmp.5.0, i64* %4, align 8 ; strides %.18.repack11 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 8 %5 = bitcast [6 x i8]* %.18.repack11 to i64* store i64 %arg.tmp.6.0, i64* %5, align 8 ; CC return store i8* null, i8** %retptr, align 8 ret i32 0 }
i.e. record which ends up as a block of bytes being read as an array model?
@stuartarchibald @gmarkall from I can see in the numba IR, the variables are correcly typed as
nestedarray. TheNestedArrayModelinherits from theArrayModelso it makes sense that it's being used.When I made the original changes I noticed that nestedarray were using the array model but in the examples I looked at, the members like nitems, shape, strides, etc were being inserted in hard-coded way like this
store <2 x i64> <i64 3, i64 2>, <2 x i64>* %1, align 8, and only thedatamember was being copied from the nestedarray into the array. This happens in an example like thisdef set1(dest): # dest is a record, not a record array, the field array1 is a nested array return dest['array1']
In this case I can see the
dataofdestbeing copied into thedataof%retptr, while the other fields of%retptrare filled directly.However, in a case like this
def set1(index, src, dest): dest['array1'] = src[index]['array']
the whole LLVM struct of src[index]['array']
(ie nitem, strides, data, etc, etc) is being copied intodest['array1']`. Could this be an issue in the lowering of staticsetitem? Maybe it needs to be modified to treat nestedarray different from array. I'll look into this later today.As an aside to help me understand the code, do you know what the "fe" and "be" mean in
fe_typeandbe_type? Like in the example below@register_default(types.NestedArray) class NestedArrayModel(ArrayModel): def __init__(self, dmm, fe_type): self._be_type = dmm.lookup(fe_type.dtype).get_data_type() super(NestedArrayModel, self).__init__(dmm, fe_type)
As an aside to help me understand the code, do you know what the "fe" and "be" mean in
fe_typeandbe_type? Like in the example belowFE is frontend - the Numba types like
numba.types.uint32, etc., and BE is backend - LLVM types likellvmlite.ir.types.IntType(32)- the data models provide the mapping between the frontend types and the backend types.update: I've tracked the issue to this function
Lines 2589 to 2606 in bf480b9
The logic is fine for assignment of scalars into scalar fields, but with arrays into arrays this happens:
thedptrvariable has value%".74" = bitcast i8* %".73" to {i8*, i8*, i64, i64, i16*, [1 x i64], [1 x i64]}*. This a problem because%".73"is defined as%".73" = getelementptr inbounds [16 x i8], [16 x i8]* %"arg.dest", i32 0, i32 8.
So%".73"is the pointer to the data of the nestedarray, but%".74"is the wrong bitcast, to the structure that represents the full data model, not just the dataI have a very ugly poc that extracts the data of the source and stores it in the record field. #7712
I'm pretty sure that the resulting LLVM IR is not good, and I probably didn't follow established patterns in numba of how to do this. For example, is it ok to call
builder.storedirectly fromrecord_setattror should I have done that viacontext?On the plus side it works, so at least I know it is a step in the right direction.
Reacted by nelson2005- added a commit that references this issue
on Jan 7, 2022
This works as expected in plain python but is incorrect in 0.55.0rc1 on Windows 10 with python 3.7.1
Correct plain python result: (1.2, [1, 2, 3])
Incorrect njit result: (1.2, [0, 0, 0])
This was first discussed in discourse using 0.53.1 with a different manifestation. Shout-out to @HPLegion for suggesting trying the latest release candidate.