Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Call new deserializer backend for constrained reads #856
Call new deserializer backend for constrained reads #856
Changes from 5 commits
66bf994
8623456
6ce419d
9f6836b
d045c45
03e789e
ce092c3
c28d4ab
7a601ac
0e0fecb
3eaab9e
1716dff
5191df7
fae0929
4c9fdaf
9d81520
d4895b1
7b9cf1c
4a846cf
c76af48
854c78e
e66e7db
aa5c9bc
75eba6a
ef05433
2a531d1
53115a7
b4ad3ae
4696379
5591542
87ba3f1
9d511d5
af670fa
171edb3
21154d4
38ba10b
b8e666e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think up here for not having
lp__
for plain readsdeserializer.read<>(dims...)
you want to check ifconstrain_string
is blank (or if constrain_args is size 0? either or) and if not then don't includelp__
as an argThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for here @bbbales2 and I are talking about changing the methods names to
to match the signature for the read free functions
But that would just involve changing this line to
We should have that sorted by today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, lemme know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding an expect test for this Transform_mir step? I just realized it's only kind of implicitly tested and I'd really love to see the actual reified MIR before and after to help me see how this step works. It doesn't need to be super complicated but it'd be good if it had a constrained parameter. A thing that would be great would be picking some example model and adding a test that just runs the compiler up to right before the Transform_mir step, prints that out, then runs Transform_mir and prints it out again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be much simpler now, the MIR arguments just reflect how they're called in codegen. It's lousy that I added a an lp__ variable in the MIR but it's better than it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these have to go through assigns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid it but idk how. Since we are guaranteed that the deserializer has enough values in it for each parameter this could be
The assignment is generated here. Would we need to change that
Fixed.pattern
to aDecl
? I can't figure out how to do deceleration and assignment on the same line. But doing that would save both memory and computational timeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a change that removes that, feel free to remove it if you dont like it.