This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][arith] More float op folders
ClosedPublic

Authored by csigg on Jan 26 2022, 6:27 AM.

Details

Summary

Fold arith.fadd %x, -0.0 -> %x and similarly for fsub, fmul, fdiv.

Fold arith.fmin %x, %x -> %x, arith.fmin %x, +inf -> %x and similarly for fmax.

Diff Detail

Event Timeline

csigg created this revision.Jan 26 2022, 6:27 AM
csigg requested review of this revision.Jan 26 2022, 6:27 AM
bondhugula added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
580–585

It'll be useful to as well add an m_ZeroFloat along the lines of the int m_Zero.

581–582

I think this won't work for non f32 Values. You need to create an APFloat zero with the right float semantics. All your test cases are with f32 -- could you add/try one with f16 or f64?

csigg updated this revision to Diff 403295.Jan 26 2022, 8:54 AM

Adding float value matchers and using the same pattern for int value matchers for consistency.

csigg marked an inline comment as done.Jan 26 2022, 8:57 AM
csigg added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
581–582

It did work for non f32 values. The initialization value didn't matter, but I admit initializing with 0.0f was confusing. I'm using a different c'tor now that produces a bogus value. Plus I changed some tests to use f16 and f64.

@jpienaar mentioned to me that `add %x

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
581

Actually I think that %x + 0.0 can't be folded to %x, but %x + -0.0 can!

That's because -0. + 0. == +0.0.

Seems very relevant to https://llvm.discourse.group/t/rfc-fastmath-flags-support-in-mlir-arith-dialect/6049/2

Can you add context and motivation in the description also? "Remove 'Commutative' interface from fmin/fmax " deserve to be explained, I would even say that it deserve a separate revision from adding folders on simple arithmetic.

csigg updated this revision to Diff 403506.Jan 26 2022, 10:57 PM

Addressing reviewer comments:

  • don't fold addf(x, +0) and subf(x, -0)
  • split out Commutative tag change
csigg marked an inline comment as done.Jan 26 2022, 11:04 PM

I would even say that it deserve a separate revision from adding folders on simple arithmetic.

Split out into https://reviews.llvm.org/D118318.

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
581

Thanks a lot Mehdi for catching this! I changed it to only fold x + -0, -0 + x and x - +0.

csigg updated this revision to Diff 403509.Jan 26 2022, 11:06 PM
csigg marked an inline comment as done.

Rebase

pifon2a accepted this revision.Jan 28 2022, 3:31 AM
This revision is now accepted and ready to land.Jan 28 2022, 3:31 AM
csigg updated this revision to Diff 403970.Jan 28 2022, 4:43 AM

Rebase.

mehdi_amini accepted this revision.Jan 28 2022, 12:00 PM
bondhugula accepted this revision.Jan 28 2022, 5:21 PM
csigg updated this revision to Diff 404471.Jan 31 2022, 3:19 AM

Rebase.

csigg edited the summary of this revision. (Show Details)Jan 31 2022, 3:39 AM
csigg edited the summary of this revision. (Show Details)
csigg updated this revision to Diff 404483.Jan 31 2022, 4:34 AM

Also fold minf(%x, +inf) -> %x, fix test.

csigg edited the summary of this revision. (Show Details)Jan 31 2022, 4:49 AM
csigg updated this revision to Diff 404489.Jan 31 2022, 5:08 AM

Simplify 1.0 matcher, add test for minf(inf, %x) -> %x.

csigg retitled this revision from More arith float op folders to [MLIR][arith] More float op folders.Jan 31 2022, 5:10 AM
This revision was automatically updated to reflect the committed changes.