Fix infer_type_of_indexed handling of MultiIndex - #1667
Conversation
infer_type_of_indexed mis-typed multi-indexing in two arms: * UVector, [MultiIndex _] (likewise URowVector and the complex variants) fell through to the scalar arm and returned UReal. Indexing a vector with an int array yields a vector. * UMatrix, [MultiIndex _] was grouped with the Single arms and returned UVector. A multi-index selects multiple rows, so the result is UMatrix. Same for UComplexMatrix. In addition, UMatrix, [MultiIndex _; Single _] matched no arm at all and raised an internal error instead of returning UVector the way [All; Single _] does. The rule is that Single reduces a dimension while All, Upfrom, Between, and MultiIndex all preserve it. MultiIndex now joins the All/Upfrom/Between arm for single indices, and the matrix pair arms gain [MultiIndex _; Single _] alongside [All; Single _]. Codegen ignores this type metadata, which is why the bug was latent, but Memory_patterns trusts it for the SoA/AoS analysis: a gather expression typed real makes matrix_set stop recursing, the gathered variable is left out of the demotion set, and the generated C++ can end up assigning an SoA value into an AoS slice, which does not compile.
|
Thanks. It seems lines 224, 226, and 227 implicitly rely on the wildcard index matching only I'd be more confident that this fixes all the corner cases if the final internal error arm was an exhaustive match instead of wildcard. I believe it should be | ( (UInt | UReal | UComplex | UTuple _ | UFun _ | UMathLibraryFunction)
, _ :: _ )
|(UVector | URowVector | UComplexVector | UComplexRowVector), _ :: _ :: _
|(UMatrix | UComplexMatrix), _ :: _ :: _ :: _ ->
ICE.(internal_errorf "Can't index %t" [UnsizedType.pp $ ut])
[@coverage off] |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1667 +/- ##
=======================================
Coverage 92.31% 92.32%
=======================================
Files 67 67
Lines 9972 9978 +6
=======================================
+ Hits 9206 9212 +6
Misses 766 766
🚀 New features to boost your workflow:
|
The wildcard internal error arm hid the assumption that a lone index wildcard means Single, which is how the MultiIndex bug went unnoticed. The final arm now lists the invalid shapes and the compiler checks that every combination is handled. That check forced two arms the wildcard used to swallow: a matrix with a Single row index and a range column index is a row vector, and a matrix with two range indices is a matrix.
|
Done. Making it exhaustive also forced two arms the wildcard used to swallow: matrix with a Single row and a range column is a row_vector, and matrix with two range indices is a matrix. Both used to hit the internal error. Added test cases for those. |
| | ( UMatrix | ||
| , ([(All | Upfrom _ | Between _ | MultiIndex _); Single _] | [Single _]) ) |
There was a problem hiding this comment.
This looks like it covers both x[ : ,1] and x[1]. But x[1] is the same as x[1, : ]. I believe this is a pre-existing bug. Matrix[Single] should be a row vector.
matrix[Single] returned UVector, but x[1] is x[1, :], a row vector. Same for complex matrices. The changed answers show up in the typed MIR metadata and the inline expect test. No generated code changes.
Expr.Helpers.infer_type_of_indexedmis-types multi-indexing.vector[MultiIndex]returnsreal(it falls through to the scalar arm),matrix[MultiIndex]returnsvector(it is grouped with theSinglearms), andmatrix[MultiIndex, Single]matches no arm and raises an internal error. The rule is thatSinglereduces a dimension whileAll,Upfrom,Between, andMultiIndexpreserve it.The bug is latent today because codegen ignores the type metadata. It matters for anything that trusts it:
Memory_patterns.matrix_setstops recursing at non-Eigen types, so an expression wrongly typedrealcan leave a variable out of the SoA demotion set. I hit this while working on #1666, where a compiler-built gather typedrealproduced C++ that assigned an SoA value into an AoS slice and did not compile.Ast_to_Mir.copy_indicesalso feeds arbitrary index lists into this function and could hit the internal error.The fix moves
MultiIndexinto the dimension-preserving arm and adds[MultiIndex, Single]to the matrix arms next to[All, Single]. New cases in the inline expect test cover vector, row vector, matrix, matrix-then-column, and arrays. No other test output changes.Submission Checklist
Release notes
Fixed the inferred type of multi-indexed expressions in the compiler's middle intermediate representation.
Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)
I used AI but understand the code and think it makes sense.