This is an archive of the discontinued LLVM Phabricator instance.

[mlir][math] Uplift from arith to math.fma
ClosedPublic

Authored by Hardcode84 on Jun 10 2023, 2:15 PM.

Details

Summary

Add pass to uplift from arith mulf + addf ops to math.fma if fastmath flags allow it.

Diff Detail

Event Timeline

Hardcode84 created this revision.Jun 10 2023, 2:15 PM
Herald added a project: Restricted Project. · View Herald Transcript
Hardcode84 requested review of this revision.Jun 10 2023, 2:15 PM
kuhar added a comment.Jun 10 2023, 2:34 PM

Just some nits

mlir/lib/Dialect/Arith/Utils/Utils.cpp
175–182

Could we use llvm::enum_seq to iterate over these?

mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp
34

nit: public is the default for structs. Instead, consider adding final:

struct UpliftFma final : OpRewrite...
57

nit: The type of fmt it not obvious here -- we should spell it out instead of using auto

63–64

nit: also here

mlir/test/Dialect/Math/uplift-to-fma.mlir
1

nit: this mixes short (-xyz) and long options (--xyz)

review comments

Hardcode84 marked 3 inline comments as done.Jun 10 2023, 2:49 PM
Hardcode84 added inline comments.
mlir/lib/Dialect/Arith/Utils/Utils.cpp
175–182

They are bitfields and not consecutive values, I don't this it will work.

Hardcode84 marked an inline comment as done.Jun 10 2023, 2:51 PM
kuhar accepted this revision.Jun 10 2023, 5:55 PM

The implementation looks good to me, but you may want to wait for a second approval from someone with more context

mlir/include/mlir/Dialect/Math/Transforms/Passes.td
20

I don't think we need arith here if the pass doesn't create any arith ops.

mlir/lib/Dialect/Arith/Utils/Utils.cpp
175–182

Ack

This revision is now accepted and ready to land.Jun 10 2023, 5:55 PM
tschuett added inline comments.
mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp
47

Do you really need the braces?

49

Why else if you return anyway?

tschuett added inline comments.Jun 10 2023, 10:28 PM
mlir/lib/Dialect/Arith/Utils/Utils.cpp
183

I always check (a & flag) == flag.

tschuett added inline comments.Jun 10 2023, 11:51 PM
mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp
49

Please ignore that comment. Apologies.

Hardcode84 added inline comments.Jun 11 2023, 2:50 AM
mlir/lib/Dialect/Arith/Utils/Utils.cpp
183

Actually, this entire function can be replaced by just a & b, don't know why I decided to this in a such convoluted way ))

mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp
47

Braces need to silence == vs = in condition warnings.

remove arith from dependentDialect, remove FMFIntersect fuction

Hardcode84 marked an inline comment as done.Jun 11 2023, 2:57 AM

The implementation looks good to me, but you may want to wait for a second approval from someone with more context

@kuhar @tschuett I'm going to merge this, as there are no more comments.

jpienaar accepted this revision.Jun 18 2023, 6:21 AM
jpienaar added a subscriber: jpienaar.
jpienaar added inline comments.
mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp
53

Nit: prefer rewriter.notifyMatchFailure to be able to give context during debugging as to match failures.

rebase, notifyMatchFailure

Hardcode84 marked an inline comment as done.Jun 18 2023, 7:32 AM
This revision was automatically updated to reflect the committed changes.