This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add constant folder for fptosi and friends
ClosedPublic

Authored by wsmoses on Dec 27 2021, 1:07 PM.

Details

Summary

This patch adds constant folds for FPToSI/FPToUI/SIToFP/UIToFP

Diff Detail

Event Timeline

wsmoses created this revision.Dec 27 2021, 1:07 PM
wsmoses requested review of this revision.Dec 27 2021, 1:07 PM
mehdi_amini added inline comments.Dec 27 2021, 1:48 PM
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
891

The doc for UIToFPOp says "rounded using the default rounding mode", I haven't found where is the "default rounding mode" defined?

(the FPToUI are well defined)

960

Can you add tests for the invalid case here?

wsmoses updated this revision to Diff 396343.Dec 27 2021, 2:00 PM

Add tests where conversion is not applied

wsmoses marked an inline comment as done.Dec 27 2021, 2:05 PM
wsmoses added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
891

I wasn't entirely sure either (besides a potential runtime CPU flag).

LLVM itself has a similar ambiguity (https://llvm.org/docs/LangRef.html#uitofp-to-instruction), though uses the same conversion (https://llvm.org/doxygen/ConstantFold_8cpp_source.html#l00477).

bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
910
929
955
mlir/test/Dialect/Arithmetic/canonicalize.mlir
490

Nit: please use snake case in function names to be consistent with other test cases.

wsmoses updated this revision to Diff 396360.Dec 27 2021, 7:12 PM

Address nits

bondhugula requested changes to this revision.Dec 27 2021, 7:53 PM
bondhugula added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
952

Please use camelBack. All of these lead to clang-tidy warnings with code completion engines and several editor environments masking other actionable warnings. You have numerous PascalCase variables mixed here and above.

This revision now requires changes to proceed.Dec 27 2021, 7:53 PM
wsmoses updated this revision to Diff 396367.Dec 27 2021, 8:09 PM

Address style nits

wsmoses updated this revision to Diff 396372.Dec 27 2021, 10:07 PM

Fix clang-tidy

wsmoses marked 5 inline comments as done.Dec 27 2021, 10:08 PM
bondhugula accepted this revision.Dec 28 2021, 12:08 AM

LGTM - please do wait for Mehdi's feedback before merging this is.

This revision is now accepted and ready to land.Dec 28 2021, 12:08 AM
mehdi_amini accepted this revision.Dec 28 2021, 8:37 PM
mehdi_amini added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
891

OK I dug into this, and it comes from IEEE 754. It'd be nice if this was acknowledged in the doc though...

This revision was automatically updated to reflect the committed changes.