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.

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

[Mobile] MLAS_PLATFORM reading AA64ISAR0_EL1 causes SIGILL on older Android devices #17851

Closed
john-dance opened this issue Oct 9, 2023 · 2 comments
Labels
platform:mobile issues related to ONNX Runtime mobile; typically submitted using template

Comments

@john-dance
Copy link

john-dance commented Oct 9, 2023

Describe the issue

PR #16082 introduced reading AA64ISAR0_EL1 to check dot product support. This was later reported as failing on the Apple M1 (see #16496) and fixed in PR #16763.

See platform.cpp
asm("mrs %[reg], ID_AA64ISAR0_EL1\n" : [reg] "=r"(isar0_el1) : :);

The instruction also fails on older Android Devices
Google Pixel 3 / Snapdragon 845 / Android 12
Samsung Galaxy A6 2018 / Cortex-A53 ( Armv8-A) / Android 10

The developer page for the AA64ISAR0_EL1 register seems to reference v8.2 for the valid values that the code is checking.

https://developer.arm.com/documentation/ddi0601/2023-09/AArch64-Registers/ID-AA64ISAR0-EL1--AArch64-Instruction-Set-Attribute-Register-0

This code path needs to be changed for older ARM devices in addition to the Apple M1.

To reproduce

Run a sample that initializes MLAS_PLATFORM on an older device such as the Pixel 3 or Samsung Galaxy A6.

Urgency

No response

Platform

Android

OS Version

Failed on both Android 10 and Android 12

ONNX Runtime Installation

Built from Source

Compiler Version (if 'Built from Source')

n/a - code is assembly

Package Name (if 'Released Package')

None

ONNX Runtime Version or Commit ID

1.16.0

ONNX Runtime API

C++/C

Architecture

X64

Execution Provider

Default CPU

Execution Provider Library Version

n/a

@john-dance john-dance added the platform:mobile issues related to ONNX Runtime mobile; typically submitted using template label Oct 9, 2023
@natke
Copy link
Contributor

natke commented Oct 9, 2023

Thank you for reporting this @john-dance. We are aware of the issue and have a fix in progress.

skottmckay added a commit that referenced this issue Oct 12, 2023
### Description
<!-- Describe your changes. -->
Use cpuinfo value when checking to dot product is available. Reading the
ID_AA64ISAR0_EL1 register is unsafe.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->
#17647 
#17541 
#17851
@edgchen1
Copy link
Contributor

Fixed by #17885

snnn pushed a commit to snnn/onnxruntime that referenced this issue Nov 1, 2023
### Description
<!-- Describe your changes. -->
Use cpuinfo value when checking to dot product is available. Reading the
ID_AA64ISAR0_EL1 register is unsafe.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->
microsoft#17647 
microsoft#17541 
microsoft#17851
kleiti pushed a commit to kleiti/onnxruntime that referenced this issue Mar 22, 2024
### Description
<!-- Describe your changes. -->
Use cpuinfo value when checking to dot product is available. Reading the
ID_AA64ISAR0_EL1 register is unsafe.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->
microsoft#17647 
microsoft#17541 
microsoft#17851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:mobile issues related to ONNX Runtime mobile; typically submitted using template
Projects
None yet
Development

No branches or pull requests

3 participants