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

Default-initialized tuples of arrays aren't currently supported #17050

Open
bradcray opened this issue Jan 29, 2021 · 7 comments
Open

Default-initialized tuples of arrays aren't currently supported #17050

bradcray opened this issue Jan 29, 2021 · 7 comments

Comments

@bradcray
Copy link
Member

bradcray commented Jan 29, 2021

Summary of Problem

[this is an update to issue #15759, whose behavior has improved from a segfault to a user-facing error]

Chapel currently doesn't support default initialization of tuples of arrays. This is something that we should ultimately support for the sake of orthogonality. I originally ran into this issue while looking into @dataPulverizer's https://github.com/dataPulverizer/KernelMatrixBenchmark effort. At that time, these patterns resulted in segfaults, where now they result in the execution-time error:

default initialization of tuple containing array or domain type not yet implemented

This is definitely an improvement, but having them work would be even better.

Steps to Reproduce

Associated Future Test(s):
test/arrays/bugs/arrOfTupleOfArr2.chpl #17051
test/arrays/bugs/arrOfTupleOfArr3.chpl #17051
test/arrays/bugs/arrOfTupleOfArr4.chpl #17051
test/arrays/bugs/arrOfTupleOfArr5.chpl #17051

Configuration Information

  • Output of chpl --version: chpl version 1.24.0 pre-release (a721b8db24)
bradcray added a commit to bradcray/chapel that referenced this issue Jan 29, 2021
…t work

This is a capture of the issue originally report in issue chapel-lang#15759,
though the behavior is now improved (in that it generates a
user-facing error rather than a segfault), leading to chapel-lang#17050.

---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
@bradcray
Copy link
Member Author

@mppf / @dlongnecke-cray: I was originally optimistic that David Longnecker's PR this week would improve the situation for these patterns as well (since they originated from a user), but no such luck. I was curious whether it was clear to either of you what would be required to resolve them and whether it's a heavy or light lift. Thanks.

bradcray added a commit that referenced this issue Jan 30, 2021
Add futures showing default init of tuples of arrays doesn't work

[trivial, not reviewed]

This is a capture of the issue originally report in issue #15759
in the form of future tests, though the behavior is now improved
(in that it generates a user-facing error rather than a segfault),
leading to #17050.
@dlongnecke-cray
Copy link
Contributor

dlongnecke-cray commented Jan 30, 2021

It's crazy to me that there was already a compiler error (in the form of inserting a runtime error) for this case, and we just didn't hit it because we were segfaulting instead.

I'm going to have to learn a bit more about array runtime types to contribute to this effort.

Conjecture:

From reading lowerRuntimeTypeInit in the compiler, it seems like the problem is that the tuple _defaultOf has no way to access the RTT of its array elements, and thus can't default initialize them.

So it seems like we'd have to generate a second _defaultOf or the like that is passed the runtime type as an argument to use when initializing the tuple/record?

@bradcray
Copy link
Member Author

It's crazy to me that there was already a compiler error (in the form of inserting a runtime error) for this case, and we just didn't hit it because we were segfaulting instead.

I assumed that the compiler error was added after the segfault was filed (since I never filed a future to track the behavior), but admittedly, I didn't compare the timestamps.

@mppf
Copy link
Member

mppf commented Jan 30, 2021

Right, the compiler doesn't currently track runtime types for type fields of aggregates. For example, in record R { type t; }, it could be instantiated with an array - but the record type itself won't contain a runtime type. It has been this way for a very long time. We have a bunch of other issues about this - #15929 #11549.

Of course, we can try to plug the issue by making specific fixes (e.g. in this case, have the compiler track the runtime type information for the tuple and pass that to defaultOf) but I think the more principled solution is to adjust the compiler to start considering types containing runtime types to have runtime types. Barring that, #11549 proposes that we at least make these cases into compilation errors.

@dlongnecke-cray
Copy link
Contributor

but I think the more principled solution is to adjust the compiler to start considering types containing runtime types to have runtime types

I was just writing a post to say after thinking about it a bit more, this is the only thing that makes sense to do!

E.g.

proc foo() type {
  return ([0, 1, 2], [0, 1, 2]);
}

// The only way foo() can communicate a dynamic type is if it has one as well, right?
var t: foo();

@bradcray
Copy link
Member Author

I agree that this is the way, thanks for explaining the situation Michael. In ancient history, there was a routine in module code that generated the runtime type for a given type (and it was used in a weird way by the compiler to generate both type and value information). Is that still the case do you know? If so, I wonder whether we can just have the compiler generate more overloads of it or write a fancy version of it which uses Reflection to iterate over fields, recurse, etc. (that'd be a stress test for reflection...).

@mppf
Copy link
Member

mppf commented Jan 30, 2021

Is that still the case do you know?

Yes

If so, I wonder whether we can just have the compiler generate more overloads of it

I think it's probably going to go better to have the compiler directly support such runtime types but you could say I am biased in that direction. But anyway, yes, we could conceptually think about generating those same functions.

cassella added a commit to cassella/chapel that referenced this issue Jan 12, 2023
This test for chapel-lang#7716 now gives a reasonable compilation error instead
of a segfault.  In the mean time, since chapel-lang#17050 added futures
test/arrays/bugs/arrOfTupleOfArr[2-6] covering this same error, remove
this one instead of adding a .bad file.

---
Signed-off-by: Paul Cassella <gitgitgit@manetheren.bigw.org>
benharsh added a commit that referenced this issue Jan 12, 2023
Remove tuple-with-array-1 future as a dup of 17050

This test for #7716 now gives a reasonable compilation error instead
of a segfault. In the mean time, since #17050 added futures
test/arrays/bugs/arrOfTupleOfArr[2-6] covering this same error, remove
this one instead of adding a .bad file.

[r @benharsh]

Merging on behalf of @cassella
ct-clmsn pushed a commit to ct-clmsn/chapel that referenced this issue Feb 11, 2023
This test for chapel-lang#7716 now gives a reasonable compilation error instead
of a segfault.  In the mean time, since chapel-lang#17050 added futures
test/arrays/bugs/arrOfTupleOfArr[2-6] covering this same error, remove
this one instead of adding a .bad file.

---
Signed-off-by: Paul Cassella <gitgitgit@manetheren.bigw.org>
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

3 participants