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 resolve tuple 'this' #25006

Merged
merged 12 commits into from
Jul 3, 2024
Merged

Conversation

riftEmber
Copy link
Member

@riftEmber riftEmber commented May 7, 2024

Resolve tuple this calls (with non-param index values) in Dyno.

This is implemented by wiring up Dyno's TupleType to ChapelTuple._tuple in the bundled modules. The module implementation of _tuple.size (called by this) is technically used but its param value is computed by dyno, since in production it is only set later during generic instantiation.

Depends on: #25007, #25102, #25231, #25315.

Resolves https://github.com/Cray/chapel-private/issues/6104.

[reviewer info placeholder]

Testing:

  • dyno tests
  • paratest
  • reproducer from backing issue now resolves

@riftEmber riftEmber marked this pull request as draft May 7, 2024 19:08
@riftEmber riftEmber changed the title Implement PRIM_GET_SVEC_MEMBER* in Dyno dyno resolve tuple 'this' May 7, 2024
@riftEmber riftEmber force-pushed the dyno-tuple-this branch 7 times, most recently from 4ed2380 to 814c975 Compare May 14, 2024 15:35
@riftEmber riftEmber changed the title dyno resolve tuple 'this' Dyno resolve tuple 'this' May 23, 2024
@riftEmber riftEmber force-pushed the dyno-tuple-this branch 5 times, most recently from 4513cac to 7d8f70a Compare May 28, 2024 19:15
@riftEmber riftEmber force-pushed the dyno-tuple-this branch 2 times, most recently from 57aba2e to 26253c6 Compare May 31, 2024 16:16
@riftEmber riftEmber force-pushed the dyno-tuple-this branch 4 times, most recently from f776f2e to 9b9ecfa Compare June 18, 2024 18:16
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
In favor of solution from
chapel-lang#25190

Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
Signed-off-by: Anna Rift <anna.rift@hpe.com>
@riftEmber riftEmber marked this pull request as ready for review July 2, 2024 17:57
Copy link
Contributor

@dlongnecke-cray dlongnecke-cray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That we can lookup methods just by hooking up the module _tuple ID is too cool!

@riftEmber riftEmber merged commit 5911c2b into chapel-lang:main Jul 3, 2024
7 checks passed
@riftEmber riftEmber deleted the dyno-tuple-this branch July 3, 2024 21:02
@bradcray
Copy link
Member

bradcray commented Jul 3, 2024

@riftEmber : I have a potentially naive question, with the caveat that I haven't been following this effort in any detail. When you say:

Resolve tuple this calls (with non-param index values) in Dyno.

Is this for homogeneous tuples only? I ask because when indexing a heterogeneous tuple with a non-param value, it seems like we wouldn't be able to get far since we wouldn't know what type it was. E.g.,

const tup = (1, 2.3, "four");
const idx = genRandInt(0..<tup.size);
f(tup(idx));  // can't determine argument type of f()

proc f(x: int) { … }
proc f(x: real) { … }
proc f(x: string) { … }

@riftEmber
Copy link
Member Author

riftEmber commented Jul 3, 2024

Is this for homogeneous tuples only?

Yes it is, I could have been more specific. For a heterogeneous tuple this with non-param would be a compiler error (https://github.com/chapel-lang/chapel/blob/4061919/modules/internal/ChapelTuple.chpl#L167-L168). I specified non-param since the param case is already handled in the compiler in dyno.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants