-
Notifications
You must be signed in to change notification settings - Fork 26k
Fix DWConv in QNNPACK for aarch32 #151191
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
Fix DWConv in QNNPACK for aarch32 #151191
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/151191
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9c7d4dd with merge base 3f0931b ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
330acb8 to
9c7d4dd
Compare
| PUSH {r0, r3, r4, r5, r6, r7, r8, r9, r10, r11, lr} | ||
| VPUSH {d8-d15} | ||
|
|
||
| STR r0, [sp, #-8] |
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.
Aren't we clobbering d8, unintentionally?
This is done elsewhere too, unfortunately, do you want to take a stab at other kernels too>
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.
Aren't we clobbering d8, unintentionally?
No, the r* registers are pushed and then the d*, all above the SP. The reading offsets for r0 and r3 are adjusted accordingly.
This is done elsewhere too, unfortunately, do you want to take a stab at other kernels too>
We didn't find additional issues in our QNNPACK version, in files such as 8x8-aarch64-neon.S and 8x8-aarch64-neon.S: contrary to STR r0, [sp, #-8] (sp is not modified), instructions such as STP d15, d14, [sp, -16] pre-modify sp. (So values are kept safe over the stack pointer.)
|
Dear all, |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
|
Dear jerryzh168, salilsdesai, kimishpatel, digantdesai, and jianyuh The described bug was confirmed, patched, and the fix was tested and passes all the CI tests. |
|
CC @jerryzh168, @salilsdesai, @kimishpatel, @digantdesai, and @jianyuh |
Some function arguments are stored below the stack pointer, that is, in a free memory area. Any call that stores values in the SP (e.g. OS context switch) will corrupt these values after return. We didn't face this problem in Linux, but it raises an
_ARMV4_Exception_data_abort_defaultin RTEMS.cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @jerryzh168