This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Math] Add constant folder for CosOp.
ClosedPublic

Authored by jacquesguan on Aug 4 2022, 8:04 PM.

Details

Summary

This patch adds constant folder for CosOp which only supports single and double precision floating-point.

Diff Detail

Event Timeline

jacquesguan created this revision.Aug 4 2022, 8:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 8:04 PM
jacquesguan requested review of this revision.Aug 4 2022, 8:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 8:04 PM
ftynse added inline comments.Aug 5 2022, 2:24 AM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
360–376

I don't think this test should be affected

Mogball requested changes to this revision.Aug 11 2022, 8:39 AM
Mogball added inline comments.
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
360–376

+1 can you please look into this? Is it an issue with the C++ cos function?

This revision now requires changes to proceed.Aug 11 2022, 8:39 AM
jacquesguan added inline comments.Aug 11 2022, 7:27 PM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
360–376

Yes, I think so. I tried cosf(acosf(-1.0f/2)), the result is not zero too. I think the way cosf and cos caculates might cause some difference.

I look the constant folder in llvm, it also use cos in libmath to fold llvm.cos. C cos function would cause some inaccuracy than the approximation we had. Any advice for this?

mehdi_amini added inline comments.Aug 23 2022, 2:12 AM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
358–359

Can you extra the constant in a function and call it so that folder does not kick in here?

jacquesguan added inline comments.Aug 25 2022, 11:22 PM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
358–359

It maybe a little odd to have a floating point constant here. In this situation, we need to extra all constant that is integral multiple of PI/2, and we need a new float epsilon to compare our constant input and target number.

Mogball added inline comments.Aug 30 2022, 2:49 PM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
358–359

That's been a feature request for FileCheck for a long time

360–376

I think this test needs to be changed so that the ops don't fold away. If they're just folding away, they're not testing the polynomial approximations.

jacquesguan added inline comments.Aug 30 2022, 11:28 PM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
360–376

Good idea. I do not know well about mlir's command line, how to disable
canonicalize for this test case?

Mogball added inline comments.Aug 31 2022, 11:49 AM
mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
360–376

Unfortunately, since these are folders, you'll have to "hide" the constants from the op by passing them as function arguments.

Address comment.

mlir/test/mlir-cpu-runner/math-polynomial-approx.mlir
360–376

Thanks, I changed to this form, will this be OK?

Mogball accepted this revision.Sep 6 2022, 8:59 AM

LGTM, but all the other test cases in this file will need to be changed. Since you added folders, they're folding instead of testing the pass. Can you do that in a follow-up?

This revision is now accepted and ready to land.Sep 6 2022, 8:59 AM

LGTM, but all the other test cases in this file will need to be changed. Since you added folders, they're folding instead of testing the pass. Can you do that in a follow-up?

OK.

This revision was automatically updated to reflect the committed changes.
jpienaar added inline comments.
mlir/test/Dialect/Math/canonicalize.mlir
362

Note: on Mac I'm getting

%cst = arith.constant 0.540302336 : f32

here.