Page MenuHomePhabricator

[MLIR] Replace std ops with arith dialect ops
ClosedPublic

Authored by Mogball on Sep 29 2021, 6:46 PM.

Details

Summary

Precursor: https://reviews.llvm.org/D110200

Removed redundant ops from the standard dialect that were moved to the
arith or math dialects.

Renamed all instances of operations in the codebase and in tests.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mogball requested review of this revision.Sep 29 2021, 6:46 PM

All tests are passing but I still have some cleanup to do

rriddle added inline comments.Sep 29 2021, 7:07 PM
mlir/lib/Transforms/Utils/FoldUtils.cpp
63–64

Can you use the dialect hook instead? The only reason ConstantOp is here is because the standard dialect hasn't been split yet.

Mogball added inline comments.Sep 29 2021, 8:49 PM
mlir/lib/Transforms/Utils/FoldUtils.cpp
63–64

Yeah this is part of the "cleanup" I still have to do.

Mogball updated this revision to Diff 376103.Sep 29 2021, 10:17 PM

rename arith.const -> arith.constant

rriddle requested changes to this revision.Sep 30 2021, 5:58 PM

Seems mostly mechanical, thanks!

mlir/docs/Tutorials/Toy/Ch-5.md
65–66

This likely needs an update (possibly same for Ch.6)

mlir/include/mlir/Conversion/SPIRVCommon/Pattern.h
25 ↗(On Diff #376103)

Please use the updated matchAndRewrite that takes an OpAdaptor.

mlir/lib/Analysis/AffineAnalysis.cpp
57–61

For reference, you should be able to do this.

mlir/lib/Analysis/CMakeLists.txt
39–45

It'd be really nice to remove all dialect dependencies from this target.

mlir/lib/Conversion/ArithmeticToSPIRV/ArithmeticToSPIRV.cpp
752–754

Functions should be fully namespace resolved and at the top-level.

mlir/lib/Conversion/StandardToSPIRV/StandardToSPIRV.cpp
13

In general, if you want to share pieces of code solely within the lib, you should place the include in the lib/ folder. include/ should *only* contain things that are meant to be outwardly exposed via the API, of which implementation utilities generally aren't. GPUCommon is an example: https://github.com/llvm/llvm-project/tree/main/mlir/lib/Conversion/GPUCommon

mlir/lib/Dialect/Arithmetic/IR/ArithmeticDialect.cpp
32–33
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
556–559

std::tuple<Ts...> *?

633

Please use proper ||

697

Drop the newline here.

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

Please remove usernames from TODO comments.

mlir/lib/Transforms/Utils/FoldUtils.cpp
63–64

Is this done or still pending?

mlir/test/IR/core-ops.mlir
32

Can you move these tests to a file in the Arith dialect folder?

mlir/test/IR/invalid-ops.mlir
4

Same comment as core-ops.mlir

This revision now requires changes to proceed.Sep 30 2021, 5:58 PM
Mogball marked 15 inline comments as done.Oct 1 2021, 10:10 AM
Mogball added inline comments.
mlir/lib/Dialect/Arithmetic/IR/ArithmeticDialect.cpp
32–33

Yeah this is one I need to drill myself to remember.

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
556–559

This was one "reviewer bait" because I didn't stumble across and existing example of something like this in the LLVM/MLIR codebase.

Mogball updated this revision to Diff 376566.Oct 1 2021, 10:15 AM
Mogball marked 2 inline comments as done.

Code cleanup

Mogball updated this revision to Diff 376605.Oct 1 2021, 12:27 PM

Rebase against upstream

Mogball updated this revision to Diff 376660.Oct 1 2021, 7:04 PM

Fixing bazel build and removing a stray file..

Mogball updated this revision to Diff 376676.Oct 2 2021, 12:27 AM

Defer splitting StandardTo* and *ToStandard passes until later

rriddle accepted this revision.Oct 5 2021, 11:45 AM

Did a pass through of the resolved comments, and looks good.

mlir/lib/Transforms/Utils/FoldUtils.cpp
16

Are either of these headers(Arith and Standard) necessary now?

If not can you see about closing https://llvm.org/PR48490?

rriddle added inline comments.Oct 5 2021, 11:47 AM
mlir/lib/Transforms/Utils/FoldUtils.cpp
16

Sorry linked the wrong bug, meant closing PR44866.

Mogball marked 2 inline comments as done.Oct 6 2021, 10:11 AM
Mogball updated this revision to Diff 377589.Oct 6 2021, 10:12 AM

Remove FoldUtils dependency on StandardOps/Arithmetic. Fixes PR44866

jpienaar accepted this revision.Oct 6 2021, 10:45 AM

Thanks!

This revision is now accepted and ready to land.Oct 6 2021, 10:45 AM
Mogball updated this revision to Diff 377719.Oct 6 2021, 4:14 PM
  • rebase against upstream
Mogball updated this revision to Diff 377734.Oct 6 2021, 7:18 PM
  • rename ops in flang
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 7:18 PM
Mogball updated this revision to Diff 377737.Oct 6 2021, 8:14 PM
  • clang-format
Mogball updated this revision to Diff 377936.Oct 7 2021, 11:50 AM
  • rebase against upstream because that's my life now
Mogball updated this revision to Diff 377982.Oct 7 2021, 1:01 PM
  • clang-format
Mogball updated this revision to Diff 377993.Oct 7 2021, 1:39 PM
  • update python bindings
Mogball updated this revision to Diff 379223.Oct 12 2021, 4:53 PM

updates with HEAD

Mogball updated this revision to Diff 379224.Oct 12 2021, 4:55 PM

wrong diff

Mogball updated this revision to Diff 379261.Oct 12 2021, 8:06 PM

update to HEAD

This revision was landed with ongoing or failed builds.Oct 12 2021, 8:07 PM
This revision was automatically updated to reflect the committed changes.
marbre added a subscriber: marbre.Oct 28 2021, 11:39 AM

I recently noticed that arith::ConstantOp support was added to the Cpp emitter with this commit. @jpienaar and @mehdi_amini, do we really want to add support for such ops to the printer? The alternative would be to add conversions, like the arith.constant to emitc.constant one which I have implemented in our mlir-emitc repo: https://github.com/iml130/mlir-emitc/blob/c3b7bf093417ecd50e4b246ffd85079dbd75e359/lib/Dialect/EmitC/Conversion/ArithToEmitC.cpp#L27-L38.

I recently noticed that arith::ConstantOp support was added to the Cpp emitter with this commit. @jpienaar and @mehdi_amini, do we really want to add support for such ops to the printer? The alternative would be to add conversions, like the arith.constant to emitc.constant one which I have implemented in our mlir-emitc repo: https://github.com/iml130/mlir-emitc/blob/c3b7bf093417ecd50e4b246ffd85079dbd75e359/lib/Dialect/EmitC/Conversion/ArithToEmitC.cpp#L27-L38.

I wasn't sure in what ways arith.constant superseded std.constant in EmitC. I had assumed that the C printer was wholly handling std.constant and not a subset of its functionality.