Page MenuHomePhabricator

Implement constant folding for PowFOp
Needs ReviewPublic

Authored by jacksonfellows on Jan 20 2021, 4:22 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Add a constant folder for PowFOp. Analogous to existing folders for
floating point operators, but instead of using an APFloat method to
perform its operation it first converts the operands to doubles, calls
the built-in pow function, and then converts the result back into the
proper floating point type. This is necessary since APFloat lacks a
pow method. This behavior matches how constant folding is implemented
for the pow intrinsic in
LLVM (https://github.com/llvm/llvm-project/blob/689de5841c1c4c9b0fe711b61d26f7425cf99423/llvm/lib/Analysis/ConstantFolding.cpp#L2373).

Diff Detail

Event Timeline

jacksonfellows created this revision.Jan 20 2021, 4:22 PM
jacksonfellows requested review of this revision.Jan 20 2021, 4:22 PM

Can you add a lit test for this?

Can you add a lit test for this?

Should I add it to this file: https://github.com/llvm/llvm-project/blob/main/mlir/test/Transforms/constant-fold.mlir?
Also should the cases mirror what already exists?

rriddle added inline comments.Jan 20 2021, 4:41 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2276

How does this work for floating point types with greater than 64 bits? e.g. FP80/FP128/etc.

As far as I can tell they would lose precision on the conversion. It seems like LLVM avoids this by only folding halfs, floats, and doubles. We could do the same by checking the operand types first, which is probably the right solution.

Since MLIR is also handling tensors and vectors, I'm not entirely sure how this check would be implemented (i.e. the operands can't always be cast to FloatAttr). The function constFoldBinaryOp currently handles these cases, so we'd have to re-implement some of that logic. Does someone with more experience with the existing folders have any good ideas about this?

rriddle added a comment.EditedJan 20 2021, 5:20 PM

Since MLIR is also handling tensors and vectors, I'm not entirely sure how this check would be implemented (i.e. the operands can't always be cast to FloatAttr). The function constFoldBinaryOp currently handles these cases, so we'd have to re-implement some of that logic. Does someone with more experience with the existing folders have any good ideas about this?

Can you just add a check before calling constFoldBinaryOp that the result type of the powf when fed to mlir::getElementTypeOrSelf is one of the valid types?

Add check to only fold the pow of halfs, floats, and doubles.

This matches the behavior present in LLVM.

Add lit tests for powf constant folding.

Thanks for adding the folding!

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2276

I think we can include BF16 here as well.

2277

I wouldn't say "following LLVM" here. The limitation is due to how the folding is implemented, i.e. it goes to double which naturally can't support larger representations.

2281

This comment shouldn't be necessary given that the element types are already guaranteed to be the same.

Add BF16 to allowed FP types