This is an archive of the discontinued LLVM Phabricator instance.

[mlir][math] Added basic support for IPowI operation.
ClosedPublic

Authored by vzakhari on Jul 14 2022, 3:24 PM.

Details

Summary

The operation computes pow(b, p), where 'b' and 'p' are signed integers
of the same width. The result's type matches the operands' type.

Diff Detail

Event Timeline

vzakhari created this revision.Jul 14 2022, 3:24 PM
vzakhari requested review of this revision.Jul 14 2022, 3:24 PM
vzakhari updated this revision to Diff 445027.Jul 15 2022, 9:43 AM
vzakhari updated this revision to Diff 445347.Jul 17 2022, 1:54 PM

Would math.powi or math.powsi be a better name? Is it OK to move the folder and strength reduction to dependent patches?

Would math.powi or math.powsi be a better name? Is it OK to move the folder and strength reduction to dependent patches?

Thank you for looking into this!

There is D129811, where I try to add FPowSI, so I want to distinguish power operations with integer and floating bases. They are somewhat different in behavior, e.g. FPowSI may have some floating point specific effects (like using rounding mode, trapping FP exceptions), whereas IPowSI does not have any FP-related effects.

Sure, I can split it into three patches: basic, folder and strength reduction. Please let me know if you have major concerns about this patch and D129811. If you do not have any, I will proceed with the splitting.

Would math.powi or math.powsi be a better name? Is it OK to move the folder and strength reduction to dependent patches?

Thank you for looking into this!

There is D129811, where I try to add FPowSI, so I want to distinguish power operations with integer and floating bases. They are somewhat different in behavior, e.g. FPowSI may have some floating point specific effects (like using rounding mode, trapping FP exceptions), whereas IPowSI does not have any FP-related effects.

Ahh OK.

From what I understand, looking at other operations in the arithmetic dialect, we specify the signed semantics as part of the operation name only if we want to distinguish the signed and unsigned behaviour (like arith.divsi vs. arith.divui). In this case, we always want a signed semantics so can't we just call it fpowi, ipowi, just like arith.addi? Or did I miss a point?

I see that LLVM IR does not have an equivalent to the ipowsi instruction. Could this be due to C/C++ not having the exponentiation operation in the language or in the math library? I wonder what other languages that lower to llvm IR do.

You might want to use the Depends on <phabricator-patch-number> syntax to mark dependencies between the fpowsi and ipowsi patch.

Would math.powi or math.powsi be a better name? Is it OK to move the folder and strength reduction to dependent patches?

Thank you for looking into this!

There is D129811, where I try to add FPowSI, so I want to distinguish power operations with integer and floating bases. They are somewhat different in behavior, e.g. FPowSI may have some floating point specific effects (like using rounding mode, trapping FP exceptions), whereas IPowSI does not have any FP-related effects.

Ahh OK.

From what I understand, looking at other operations in the arithmetic dialect, we specify the signed semantics as part of the operation name only if we want to distinguish the signed and unsigned behaviour (like arith.divsi vs. arith.divui). In this case, we always want a signed semantics so can't we just call it fpowi, ipowi, just like arith.addi? Or did I miss a point?

Yes, I just wanted to make clear that the power operand is signed in the operation name. I can use just 'i' instead of 'si', since I do not expect that we will need a version with unsigned power operand.

I see that LLVM IR does not have an equivalent to the ipowsi instruction. Could this be due to C/C++ not having the exponentiation operation in the language or in the math library? I wonder what other languages that lower to llvm IR do.

Yes, math.h does not define pow functions with integer power argument as well as integer base argument. C++ does have std::pow overloads with int32 power argument, but they are implemented via math.h's double precision pow (so I think their performance is not as good as it could have been if there was a specialized version as in libgcc). Not sure what the other languages do.

You might want to use the Depends on <phabricator-patch-number> syntax to mark dependencies between the fpowsi and ipowsi patch.

Thanks for the hint! I usually use web interface to add parent revisions. In this case I have a chain of 4 revisions that I was writing one after another, so I just used linear chain: D129809->D129810->D129811->D129812

I think this stack of patches deserves a quick RFC on the forum.

I think this stack of patches deserves a quick RFC on the forum.

Thanks! Created RFC: https://discourse.llvm.org/t/rfc-adding-more-power-operations-into-math-dialect/63975

vzakhari updated this revision to Diff 446938.Jul 22 2022, 12:26 PM
vzakhari retitled this revision from [mlir][math] Added basic support for IPowSI operation. to [mlir][math] Added basic support for IPowI operation..

@kiranchandramohan, @ftynse thank you for your comments!

I split this patch into 3 pieces. New parts are in D130389 and D130390.

I will be unavailable next week, so I will not be able to reply. Sorry for the inconvenience.

vzakhari updated this revision to Diff 449427.Aug 2 2022, 1:59 PM

rebase

Please review.

jfurtek accepted this revision.Aug 9 2022, 9:31 AM
This revision is now accepted and ready to land.Aug 9 2022, 9:31 AM
This revision was automatically updated to reflect the committed changes.