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

[MINOR] Enable shape inference for frame type #626

Merged

Conversation

DamianDinoiu
Copy link
Contributor

  • DAPHNE support shape inference for frame type but the feature was disabled.
  • The logic should be further revised as it is redundant to throw an error.

   - DAPHNE support shape inference for frame type but the feature was disabled.
   - The logic should be further revised as it is redundant to throw an error.
@pdamme pdamme self-requested a review October 27, 2023 19:49
Copy link
Collaborator

@pdamme pdamme left a comment

Choose a reason for hiding this comment

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

Hi @DamianDinoiu, thanks for identifying this bug and providing a fix! This pull request reveals that shapes inferred for frames were not actually set at all, such that the shapes stayed unknown in the IR. I can confirm that your proposed change solves this problem.

On top of your changes, I merely formatted the code a bit for better readability. I also added the same fix for sparsity inference; even though we do not infer sparsity for frames yet, the same problem could pop up there in the future (and the code also checked for FrameType).

Part of the reason why we didn't discover this bug before may be that we hardly actually used the inferred shapes of frames so far. Therefore, it's currently not straightforward to add a test case (but that will become easier in the future). For now, one can check the behavior by executing this simple DaphneDSL script with --explain property_inference:

print(createFrame([1]));

Before your fix, the IR after property inference still indicated an unknown shape for the frame:

%5 = "daphne.createFrame"(%4, %2) : (!daphne.Matrix<1x1xsi64>, !daphne.String) -> !daphne.Frame<?x[?: si64], ["col_0"]>

With your changes, the shape is known in the IR:

%5 = "daphne.createFrame"(%4, %2) : (!daphne.Matrix<1x1xsi64>, !daphne.String) -> !daphne.Frame<1x[1: si64], ["col_0"]>

@pdamme pdamme merged commit daf7067 into daphne-eu:main Oct 27, 2023
2 checks passed
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.

2 participants