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
feat[next]: as_offset implementation in embedded #1397
base: main
Are you sure you want to change the base?
feat[next]: as_offset implementation in embedded #1397
Changes from 31 commits
5b6b8b7
81c0141
1acb1d8
b94e81c
f815712
67f8117
e17ff41
0e61be2
0219e72
762d7eb
fa1c588
38c052c
e8d6e5e
782375b
654b14d
186b81d
e50eb64
09a4c44
9f2bcc6
90f6796
b19ba78
c5464f3
7c3e9cb
2ea83f1
6387049
1e892b4
2f3d9f6
b941b92
81fc838
7261ae7
ba1a91f
e074b10
ea7f20f
c483a0f
b363cc2
391508b
c668dc8
83a4b38
7e26c20
8bc6725
e3e8db8
5b6e553
dacc6d5
34fdeb7
84abf3d
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.
Why the second part of this condition?
restricted_connectivity_domain == new_domain
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.
to avoid entering this condition in cases like:
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.
can you explain this if else branch and are you sure all cases are handled? I am confused...
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.
When using FieldOffsets, only the specific dimensions related to the offset are taken into account.
Say I have this field_operator:
Here the
restricted_connectivity_domain
will be over[Edge, V2E]
and will excludeKDim
. In this case using the regularxp.take
works.When using
as_offset
,xp.take
is also ok to use if theoffset_field
contains only one dimension.However, when
restricted_connectivity_domain
contains multiple dimensions that are exactly the same as innew_domain
, we have seen thatxp.take
does not work and hence had to create_take_mdim
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.
but what about
restricted_connectivity_domain.dims == new_domain.dims
, but ranges are different?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.
why are the changes in this file needed?
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.
as_offset
was breaking with the dace backend. These changes were discussed with @edopao and @petiaccjaThe dace backend does not consider
offset_dim
to compute the key to access the build cacheThere 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 have included this change in one of my PRs, which I have already merged. So you won't need to do any change in this file after you rebase (but you'll get a rebase conflict).