Add pass to uplift from arith mulf + addf ops to math.fma if fastmath flags allow it.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Just some nits
| mlir/lib/Dialect/Arith/Utils/Utils.cpp | ||
|---|---|---|
| 175–182 ↗ | (On Diff #530252) | Could we use llvm::enum_seq to iterate over these? |
| mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp | ||
| 35 | nit: public is the default for structs. Instead, consider adding final: struct UpliftFma final : OpRewrite... | |
| 58 | nit: The type of fmt it not obvious here -- we should spell it out instead of using auto | |
| 64–65 | nit: also here | |
| mlir/test/Dialect/Math/uplift-to-fma.mlir | ||
| 2 | nit: this mixes short (-xyz) and long options (--xyz) | |
| mlir/lib/Dialect/Arith/Utils/Utils.cpp | ||
|---|---|---|
| 175–182 ↗ | (On Diff #530252) | They are bitfields and not consecutive values, I don't this it will work. |
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 ↗ | (On Diff #530252) | Ack |
| mlir/lib/Dialect/Arith/Utils/Utils.cpp | ||
|---|---|---|
| 183 ↗ | (On Diff #530256) | I always check (a & flag) == flag. |
| mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp | ||
|---|---|---|
| 49 | Please ignore that comment. Apologies. | |
| mlir/lib/Dialect/Math/Transforms/UpliftToFMA.cpp | ||
|---|---|---|
| 52 | Nit: prefer rewriter.notifyMatchFailure to be able to give context during debugging as to match failures. | |
I don't think we need arith here if the pass doesn't create any arith ops.