Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix a bug in WASM's GEMM #20023

Merged
merged 2 commits into from
Mar 23, 2024
Merged

Fix a bug in WASM's GEMM #20023

merged 2 commits into from
Mar 23, 2024

Conversation

snnn
Copy link
Member

@snnn snnn commented Mar 22, 2024 •
edited
Loading

Description

Fix a bug in WASM's GEMM. The bug was found when running "ConvAddActivationFusionTests.ConvGemmDirect" unit test in a wasm build with address sanitizer enabled. When CountK=25, CountN=1, lda=25, ldc=1, the function I am modifying triggered a read out of bound error.

The bug fix was provided by @fs-eire.

@snnn snnn requested a review from a team as a code owner March 22, 2024 03:26
@snnn snnn requested a review from fs-eire March 22, 2024 03:26
fs-eire
fs-eire previously approved these changes Mar 22, 2024
View reviewed changes
@fs-eire
Copy link
Contributor

fs-eire commented Mar 22, 2024

/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline

@fs-eire
Copy link
Contributor

fs-eire commented Mar 22, 2024

/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models,Android CI Pipeline

@fs-eire
Copy link
Contributor

fs-eire commented Mar 22, 2024

/azp run iOS CI Pipeline,ONNX Runtime React Native CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 10 pipeline(s).

@snnn
Copy link
Member Author

snnn commented Mar 22, 2024

/azp run Big Models, Linux OpenVINO CI Pipeline, Linux QNN CI Pipeline, MacOS CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@fs-eire
Copy link
Contributor

fs-eire commented Mar 22, 2024

@tianleiwu @yufenglee

some background of this change:

The WASM version is originally translated from arm implementation, and the specific line was originally vld1_f32

Left: https://github.com/fs-eire/onnxruntime/blob/7f968f4110cc16812a4d9b2c6fbe9a07f5801b9f/onnxruntime/core/mlas/lib/wasm_simd/sgemmc.cpp
Right: https://github.com/intel/onnxruntime/blob/89618e8f1e8fee0354de57494bb71da96a445b71/onnxruntime/core/mlas/lib/arm/sgemmc.cpp

Diff (screenshot):
image

@tianleiwu
Copy link
Contributor

@tianleiwu @yufenglee

some background of this change:

Thanks for the explanation.

tianleiwu
tianleiwu previously approved these changes Mar 22, 2024
View reviewed changes
@snnn snnn dismissed stale reviews from tianleiwu and fs-eire via 2dca0c9 March 22, 2024 23:56
@yufenglee yufenglee merged commit 3b4b99b into microsoft:main Mar 23, 2024
92 of 94 checks passed
@snnn snnn deleted the fix_wasm branch March 23, 2024 18:20
TedThemistokleous pushed a commit to TedThemistokleous/onnxruntime that referenced this pull request May 7, 2024
@snnn
Fix a bug in WASM's GEMM (microsoft#20023)
44f9d9a 
### Description
Fix a bug in WASM's GEMM. The bug was found when running
"ConvAddActivationFusionTests.ConvGemmDirect" unit test in a wasm build
with address sanitizer enabled. When CountK=25, CountN=1, lda=25, ldc=1,
the function I am modifying triggered a read out of bound error.

The bug fix was provided by @fs-eire.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yufenglee yufenglee yufenglee approved these changes

@tianleiwu tianleiwu tianleiwu left review comments

@fs-eire fs-eire fs-eire left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@snnn @fs-eire @tianleiwu @yufenglee