Math ops lowering and runtime issue

[jinchen62]

What happened?

MLIR is here.

Compilation failed after iree-convert-to-llvm pass.

error: failed to legalize operation 'math.acos' that was explicitly marked illegal

I added theses changes and it compiled, but having runtime issue.

iree/runtime/src/iree/hal/local/executable_plugin_manager.c:570: NOT_FOUND; missing 1 required executable imports: [acosf]; while invoking native function hal.executable.create; while calling import; [ 1] native hal.executable.create:0 - [ 0] bytecode module.__init:268 model.mlir:4:10; creating VM context; creating run context

I'm not sure if the changes I made is right though.

Steps to reproduce your issue

1. git clone https://github.com/nod-ai/SHARK-TestSuite.git
2. cd SHARK-TestSuite/iree_tests/onnx/node/generated/test_acos
3. iree-compile model.mlir --iree-hal-target-backends=llvm-cpu -o model.vmfb
4. iree-run-module --module=model.vmfb --device=local-sync --flagfile=test_data_flags.txt

What component(s) does this issue relate to?

No response

Version information

No response

Additional context

No response

[hanhanW]

@pashu123 please take a look at it. My guess is that we need a polynomial approximation for math.acos. Because not all the systems have libc/libm. I could be wrong, please help take a deeper look.

[pashu123]

@hanhanW After reading the issue https://github.com/iree-org/iree/issues/4717, I think libm is not the right way to go, and you're right. I created a small Python script to emulate https://stackoverflow.com/questions/28969184/is-there-an-accurate-approximation-of-the-acos-function (for testing) https://gist.github.com/pashu123/26c0c4e45b11634538a1637c115717fb and the results are

Actual: 2.1682027434402467, Approximation: 2.1682027434402467
Actual: 1.9551931012905357, Approximation: 1.9551931012905357
Actual: 1.8234765819369754, Approximation: 1.8234765819369751
Actual: 1.5707963267948966, Approximation: 1.5707963267948966
Actual: 1.318116071652818, Approximation: 1.318116071652818
Actual: 1.1863995522992576, Approximation: 1.1863995522992574
Actual: 0.9733899101495465, Approximation: 0.9733899101495465

I will go ahead and add the rewrite in the Polynomial Approximation pass.

[pashu123]

@jinchen62 Please cherry-pick this patch and try https://github.com/pashu123/llvm-project/commit/0be25e06c6773c34c8d592f07d30a767003f28b1 (this will go into iree/third_party/llvm-project). Also, remove the mathToLibm lowering pattern.

[hanhanW]

Thanks @pashu123 , what a quick fix! Please remember to send your LLVM commit out for review, thanks!

[jinchen62]

@pashu123 Thanks for your fix! Yes I got acos onnx tests working, but got a result mismatch for an asin test. Though the tests are imported from onnx by using iree-import-onnx, the expected result might not be correct. Could you check it out?

[FAILED] result[0]: element at index 7 (1.101) does not match the expected (1.10125); expected that the view is equal to contents of a view of 3x4x5xf32 expected: 3x4x5xf32=[[0.580944 0.796895 0.64696 0.57625 0.437476][0.702194 0.452914 1.10125 1.30039 0.39352][0.913628 0.557298 0.604128 1.18261 0.0710959][0.0872399 0.0202198 0.983821 0.891726 1.05523]][[1.36363 0.925894 0.479662 0.895512 0.118552][0.694395 0.143849 1.23658 0.549016 0.427571][0.267743 0.885503 0.473664 0.604601 0.0187909][0.665733 0.658708 0.664841 1.23379 0.750248]][[0.36774 0.452296 0.772086 0.0602619 0.729862][0.735068 0.211966 0.129286 0.320908 0.372248][0.606745 0.454042 1.41816 0.102223 0.210426][0.162017 0.711682 0.256081 0.485116 0.246927]] actual: 3x4x5xf32=[[0.580944 0.796895 0.64696 0.57625 0.437476][0.702194 0.452914 1.101 1.29203 0.39352][0.913626 0.557298 0.604128 1.18135 0.0710959][0.0872399 0.0202198 0.983808 0.891725 1.05514]][[1.34451 0.925892 0.479662 0.895511 0.118552][0.694395 0.143849 1.2334 0.549016 0.427571][0.267743 0.885503 0.473664 0.604601 0.0187909][0.665733 0.658708 0.664841 1.23074 0.750248]][[0.36774 0.452296 0.772086 0.0602619 0.729862][0.735068 0.211966 0.129286 0.320908 0.372248][0.606745 0.454042 1.38255 0.102223 0.210426][0.162017 0.711682 0.256081 0.485116 0.246927]]

Also, is there any chance to add support for acosh, asinh, atanh, cosh, sinh? Thanks!

[pashu123]

@pashu123 Thanks for your fix! Yes I got acos onnx tests working, but got a result mismatch for an asin test. Though the tests are imported from onnx by using iree-import-onnx, the expected result might not be correct. Could you check it out?

[FAILED] result[0]: element at index 7 (1.101) does not match the expected (1.10125); expected that the view is equal to contents of a view of 3x4x5xf32 expected: 3x4x5xf32=[[0.580944 0.796895 0.64696 0.57625 0.437476][0.702194 0.452914 1.10125 1.30039 0.39352][0.913628 0.557298 0.604128 1.18261 0.0710959][0.0872399 0.0202198 0.983821 0.891726 1.05523]][[1.36363 0.925894 0.479662 0.895512 0.118552][0.694395 0.143849 1.23658 0.549016 0.427571][0.267743 0.885503 0.473664 0.604601 0.0187909][0.665733 0.658708 0.664841 1.23379 0.750248]][[0.36774 0.452296 0.772086 0.0602619 0.729862][0.735068 0.211966 0.129286 0.320908 0.372248][0.606745 0.454042 1.41816 0.102223 0.210426][0.162017 0.711682 0.256081 0.485116 0.246927]] actual: 3x4x5xf32=[[0.580944 0.796895 0.64696 0.57625 0.437476][0.702194 0.452914 1.101 1.29203 0.39352][0.913626 0.557298 0.604128 1.18135 0.0710959][0.0872399 0.0202198 0.983808 0.891725 1.05514]][[1.34451 0.925892 0.479662 0.895511 0.118552][0.694395 0.143849 1.2334 0.549016 0.427571][0.267743 0.885503 0.473664 0.604601 0.0187909][0.665733 0.658708 0.664841 1.23074 0.750248]][[0.36774 0.452296 0.772086 0.0602619 0.729862][0.735068 0.211966 0.129286 0.320908 0.372248][0.606745 0.454042 1.38255 0.102223 0.210426][0.162017 0.711682 0.256081 0.485116 0.246927]]

Also, is there any chance to add support for acosh, asinh, atanh, cosh, sinh? Thanks!

atanh is already there. cosh and sinh are also there. So, they might just work.

[pashu123]

@jinchen62 For these kinds of tests, i.e., polynomial approximations, you should keep a threshold. It won't match the entire precision. 1.101 and 1.10125 are close, given that it's an approximation.

[pashu123]

For SHARK test suite we had set it to https://github.com/nod-ai/SHARK/blob/cf2513e7b19960628ea44d44626fca5b23887c15/tank/model_utils.py#L346

[jinchen62]

@pashu123 I got the following kind of error for acosh, asinh, atanh, cosh, sinh.

error: failed to legalize operation 'math.atanh' that was explicitly marked illegal

Could you point me out where the supports of atanh, cosh and sinh are?

[pashu123]

@pashu123 I got the following kind of error for acosh, asinh, atanh, cosh, sinh.

error: failed to legalize operation 'math.atanh' that was explicitly marked illegal

Could you point me out where the supports of atanh, cosh and sinh are?

You will find it here: https://github.com/pashu123/llvm-project/blob/0be25e06c6773c34c8d592f07d30a767003f28b1/mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp#L1656

[pashu123]

@pashu123 I got the following kind of error for acosh, asinh, atanh, cosh, sinh.

error: failed to legalize operation 'math.atanh' that was explicitly marked illegal

Could you point me out where the supports of atanh, cosh and sinh are?

You will find it here: https://github.com/pashu123/llvm-project/blob/0be25e06c6773c34c8d592f07d30a767003f28b1/mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp#L1656

You are right. I was confused between cosh and cos. So, the implementation for acosh, asinh.. is not present.

[pashu123]

@jinchen62 So, the lowering of sinh would be 1/2 * (math.exp(x) – math.exp(-x)), cosh would be 1/2 * (math.exp(x) + math.exp(-x)) and tanh = sinh/cosh. For acosh would be math.log(x + math.sqrt(x**2 - 1)). We have an example for polynomial approximation for acos and asin; adding these would be straightforward. We can keep this a starter task if someone is willing.

[pashu123]

Also, which models are they coming from? I have only seen cos and sin from LLAMAs rotary embedding.

[ScottTodd]

@jinchen62 For these kinds of tests, i.e., polynomial approximations, you should keep a threshold. It won't match the entire precision. 1.101 and 1.10125 are close, given that it's an approximation.

FYI, tolerances can be changed using the --expected_f32_threshold=0.0001f flag, source: https://github.com/iree-org/iree/blob/9f5e356d0d7c7f62995413f03f1b42f2adac942e/runtime/src/iree/tooling/comparison.cc#L21-L26, added to either individual test cases like https://github.com/nod-ai/SHARK-TestSuite/blob/main/iree_tests/onnx/node/generated/test_acos/test_data_flags.txt (for all backends, CPU/GPU/etc.) or to configs for all tests on one backend: https://github.com/iree-org/iree/blob/9f5e356d0d7c7f62995413f03f1b42f2adac942e/build_tools/pkgci/external_test_suite/onnx_cpu_llvm_sync.json#L6-L8

We haven't had enough test cases with tolerance issues for either style to get much mileage... I'm leaning towards adding the tolerances upstream in SHARK-TestSuite for the test cases we know will often use polynomial approximations (asin, acos, etc.)

[jinchen62]

@pashu123 Thanks for your help! Could you land your llvm patch?

[pashu123]

@pashu123 Thanks for your help! Could you land your llvm patch?

Sure, I will raise the PR today.

[jinchen62]

Changed the lowering of acosh, asinh, atanh, cosh, sinh.

[pashu123]

Changed the lowering of acosh, asinh, atanh, cosh, sinh.

@ jinchen62 I suggest you add these as a part of the polynomial approximation pass since many frontends may take advantage of that. Since you've added this as a part of ONNX to TORCH lowering, this will not benefit TORCH frontend.

[jinchen62]

@pashu123 Please review https://github.com/llvm/llvm-project/pull/90718

[ScottTodd]

Please keep issues open until fixes have fully landed (https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue on the PR that fixes the issue helps automate that)

[ScottTodd]

https://github.com/iree-org/iree/pull/17324 was merged with test failures (newly passing tests need XFAIL lists updated) so it was reverted in https://github.com/iree-org/iree/pull/17324. You may also want to update tolerances as discussed.

[hanhanW]

https://github.com/iree-org/iree/pull/17395 is landed, closing the issue.