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

dyno: materialize promoted and iterable expressions into arrays when assigned to variables or returned from procedures #26466

Open
DanilaFe opened this issue Jan 3, 2025 · 1 comment

Comments

@DanilaFe
Copy link
Contributor

DanilaFe commented Jan 3, 2025

In production, writing:

var A = foreach i in 1..10 do i*i;

Creates a new array A that's populated with the results of the iterable. In Dyno, currently, it gives A an IterableType (not an array). I believe converting iterables to arrays in such cases is an explicit design choice, and thus, we ought to replicate it in Dyno.

@bradcray
Copy link
Member

bradcray commented Jan 6, 2025

Note that there are three similar-but-different cases in the production compiler for this pattern. In the one, the index set of the RHS is preserved and used to establish the array's domain. Like in this case, A would be over [1..10]. In the second, when we're iterating over a domain or an array, the new array's domain matches the iterand's. In the third case—say we're invoking an iterator that yields integers read from a file until it hits EOF—the array is 1D, 0-based, and its size is only determined at the time of its creation.

I believe Vass is most familiar with the first two cases and how they are implemented / what we use today to query whether the index set of an iterable is well-known / should be treated as the domain.

DanilaFe added a commit that referenced this issue Jan 24, 2025
This PR implements saving iterators into arrays, closing
#26466.

Brad outlines a number of special cases in
#26466 (comment).
Fortunately, these special cases are all implemented in module code; as
a result, rather than making Dyno reason about the shapes / domains of
iterators, this PR simply took the route of enabling the resolution of
various code in `ChapelArray`. This included:

1. Implementing the `_iteratorRecord` type (which simply accepts any
`IterableType`). This is required to select the various `chpl__initCopy`
overloads for constructing arrays.
2. Enabling computing `_shape_` on iterators. This has two components:
actually computing the shape (which is implemented using a Dyno query),
and exposing a `_shape_` field through special casing (like `eltType` in
Dyno's `c_ptr`). Crucially, reflection was used to check if `_shape_`
was set; this meant ensuring that `name to field num` and other
primitives work correctly with "phantom" fields on iterable types.
3. Ensuring that types that are instantiations (including via their
parent types) always have an `instantiatedFrom`, since that field is
used in many places to quickly check if a type is instantiated. I
originally thought it would make more sense to stop relying on that
invariant, but have come to a decision that quick checks of
`instantiatedFrom` are most elegant.
4. Adjusting various places in the Resolver and InitResolver to support
invoking `init` on arrays, and running the arrays' constructors. This
was needed because module code calls `new _array`.
5. Fixing a bug in which parenless procedures returning values could not
be called on class types due to a mis-use of `toCompositeType` vs
`getCompositeType`.
6. Many more fixes, too numerous to be listed here with any value. Many
fixes correspond to individual commits; consider reading through the
commit list to see if anything jumps out.

## Future work
* Move `makeConst` etc to `Qualifier`
* Adjust `ArrayType` to lean on `_instance` more (@riftEmber)
* Consider using a global `QualifiedType()` to be able to return
references and avoid constructing QTs when not needed.

# Testing
- [x] dyno tests
- [x] paratest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants