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

[enhancement:DSLX:type_system] Ensure parametric env map is completely populated on instantiation #1495

Open
cdleary opened this issue Jun 19, 2024 · 2 comments
Labels
dslx DSLX (domain specific language) implementation / front-end enhancement New feature or request

Comments

@cdleary
Copy link
Collaborator

cdleary commented Jun 19, 2024

What's hard to do? (limit 100 words)

Right now we accept situations where the caller doesn't fully populate the parametric env map. For example in this program:

fn p<X: u32, Y: u32>(y: uN[Y]) -> u32 { Y }
fn f() -> u32 { p(u7:0) }

We could eagerly state that X was not populated in the parametric environment and flag that as an issue.

There is some fallout to address when we try to tighten up this invariant, which is why I'm filing an issue for it instead of fixing it directly in the related issue #1473

Current best alternative workaround (limit 100 words)

The current lenient policy is "ok" it just causes some failures at IR conversion time instead of eagerly presenting them at typechecking time as we would like.

Your view of the "best case XLS enhancement" (limit 100 words)

We are able to eagerly flag all parametric environment bindings that have not been populated when they are instantiated.

@cdleary cdleary added enhancement New feature or request dslx DSLX (domain specific language) implementation / front-end labels Jun 19, 2024
cdleary added a commit to xlsynth/xlsynth that referenced this issue Jun 19, 2024
Previously we'd get an internal error by running head-first into the
bytecode interpreter and it returning us an unbound error from its execution.

This institutes a look-before-you-leap: we look for freevars from the
parametric expression that are present in the parametric bindings, and check
that those are present in the parametric environment map. If they are not, we
provide a more precise and helpful error message.

Ideally we'd be even more aggressive about reporting parametrics that are
unbound at the point of instantiation, that is what google#1495 is for and why
there's a bit of code stubbed out with a TODO in this PR -- there is some
investigation needed for the examples that fail with that enabled, but this
change strictly moves the ball forward by resolving the issue seen in google#1473.

Fixes google#1473
One step towards google#1495
cdleary added a commit to xlsynth/xlsynth that referenced this issue Jun 19, 2024
Previously we'd get an internal error by running head-first into the
bytecode interpreter and it returning us an unbound error from its execution.

This institutes a look-before-you-leap: we look for freevars from the
parametric expression that are present in the parametric bindings, and check
that those are present in the parametric environment map. If they are not, we
provide a more precise and helpful error message.

Ideally we'd be even more aggressive about reporting parametrics that are
unbound at the point of instantiation, that is what google#1495 is for and why
there's a bit of code stubbed out with a TODO in this PR -- there is some
investigation needed for the examples that fail with that enabled, but this
change strictly moves the ball forward by resolving the issue seen in google#1473.

Fixes google#1473
One step towards google#1495

Add license header to .x file.

git-clang-format
cdleary added a commit to xlsynth/xlsynth that referenced this issue Jun 19, 2024
Previously we'd get an internal error by running head-first into the
bytecode interpreter and it returning us an unbound error from its execution.

This institutes a look-before-you-leap: we look for freevars from the
parametric expression that are present in the parametric bindings, and check
that those are present in the parametric environment map. If they are not, we
provide a more precise and helpful error message.

Ideally we'd be even more aggressive about reporting parametrics that are
unbound at the point of instantiation, that is what google#1495 is for and why
there's a bit of code stubbed out with a TODO in this PR -- there is some
investigation needed for the examples that fail with that enabled, but this
change strictly moves the ball forward by resolving the issue seen in google#1473.

Fixes google#1473
One step towards google#1495
@mikex-oss
Copy link
Collaborator

This might not be the point of your example (since the returned Y could be replaced by X), but would it make sense to flag fn p<X: u32, Y: u32>(y: uN[Y]) -> u32 { Y } for X being unused?

It doesn't currently do so, whereas it would (as kUnusedDefinition) if the variable were defined in the function body.

@hzeller
Copy link
Member

hzeller commented Jun 24, 2024

For easy reproducibility of this issue, here are the commands to run to observe the current error hitting the snag while ir conversion:

Code generation

bazel build -c opt xls/dslx/ir_convert:ir_converter_main

IR_CONVERTER=bazel-bin/xls/dslx/ir_convert/ir_converter_main
${IR_CONVERTER} /tmp/foo.x

Result

Error: INTERNAL: Not enough symbolic bindings to convert function: p; need {X, Y} got {Y}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dslx DSLX (domain specific language) implementation / front-end enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants