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
[Tensor reorder] Universal tensor transform feature, a fallback of batched transpose kernel #1419
[Tensor reorder] Universal tensor transform feature, a fallback of batched transpose kernel #1419
Changes from all commits
c52547a
60d4564
569044f
682a725
7fc0de7
b1f5c89
9573861
ca1bb57
b0c188c
57dab09
45894a7
84863c4
54d1f2e
e5f8617
4dba45c
c3c5303
b539c9c
b9e8684
c766a69
37b1926
a36ce98
923e4b3
7802205
541a1e7
45f1a6f
3cc7c61
08a9c82
3374fa6
0dfac32
e9ac702
879694f
183e728
978f8e9
ff5e47e
7845771
16dfe07
9ef53c0
aa6a09d
108b80c
47d4b3d
74a7545
86c21af
096c661
32f21b0
eff9686
c81cb86
6b9f145
ff7a441
5d223eb
acd2877
3453bba
0651536
a08f7ce
3196935
e1244fd
8652207
6e98bfb
ad66328
d97fc28
1fe9254
a3aab19
35aa269
1072ac1
2870b32
b6aa19b
1080341
24d8916
0997915
8d6f995
41f2f35
03e5c48
04f48d6
e42f13f
d0198e2
a5099b0
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 we need uint32_t here?
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.
Answer required.
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.
dim_0_ is the one of the dimension of the tensor, I think 2^31 is safe to cover its range.
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.
4GiB tensors are not rare these days. IIRC tensors use 64 bits for each dimension. Can you please change this to a suitable 64-bit type (better) or MIOPEN_THROW if dim0 exceeds MAX_UINT (at the place where 64 bits are being cut to 32 bits). Thanks.
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 wanna choose the latter solution at the moment, the reason is that 32-bit magic division used in address computation and it will take a lot of time to modify it to 64-bit while simply add a uint32_t value guarder is much easier. I'll add 64-bit type reorder as a TODO.
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 make it so that
MakeTensorReorderAttributes
returns emptystd::unique_ptr<TensorReorderAttributesBase>
when dim0 exceeds MAX_UINT? It's better than THROW. The semantic of this return value is "tensor transform is not applicable", which is indeed so. Then the caller can use different implementation of transform (if it exists).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.
Ok!
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.
Thanks for the advice, I'll add this feature in next PR later this week.