-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[GPU] Add dropout support for Ref Matmul #4494
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
Conversation
9b5d3dc to
1ac0545
Compare
|
make test |
1ac0545 to
50c012b
Compare
50c012b to
4dc9fd9
Compare
|
make test |
dzarukin
left a comment
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.
LGTM, left some minor comments.
| float po_acc = convert_float(temp); | ||
|
|
||
| #if WITH_DROPOUT | ||
| #if USE_OFFSET |
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 this should check if SEED_DATA_T is int or int64_t. If it's not, then the assumption is the offset won't be passed anyway. If it is, then use a new version and if offset wasn't specified, the kernel sets it to 0.
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.
good catch! the offset will already be set to zero if use_offset = 0, but this condition is misleading.
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.
https://github.com/uxlfoundation/oneDNN/blob/main/src/cpu/primitive_attr_postops.cpp#L304 - based on this condition use_offset is the precursor between the two philox functions used. Will stick with what I have by adding explicit conversion to seed for philox without offset. Changing condition will mean I will have to add another similar function to cater for s64 philox without seed.
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.
Alternatively you could have two versions of philox_4x32 with different type signatures so it would dispatch implicitly based on type.
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.
We strictly like to keep the code between cpu/gpu same. Additionally if I do add another philox function, it will be just repeating the same code with lesser lines.
115b8aa to
5701436
Compare
5701436 to
417e650
Compare
|
make test |
| --check-ref-impl=false | ||
|
|
||
| --attr-dropout=0.5:12345678,0.75:12345678:undef,0.25:843921:any:1238976:true | ||
| --attr-dropout=0.5:12345678,0.75:12345678:undef,0.25:843921:any:1238976:true,0.75:111786:any:121716:false |
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.
are we able to specify s32/s64 seed type from benchdnn? it might be worth verifying both if theyre still supported.
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.
So benchdnn only supports s64 as a standard. We are keeping s32 for older test-pathways. I checked the s32 datatype here by making the change here: https://github.com/uxlfoundation/oneDNN/blob/main/tests/benchdnn/dnn_types.cpp#L1449-L1454
(and all the tests are passing for harness_matmul_dropout)
417e650 to
d04c1ad
Compare
This is the GPU side of the adding s64 offset and seed support for frameworks. The CPU/Arch part was merged before holidays here: #4031