This patch add some builtin operation for the gpu.all_reduce ops.
- for Integer only: and, or, xor
- for Float and Integer: min, max
This is useful for higher level dialect like OpenACC or OpenMP that can lower to the GPU dialect.
Paths
| Differential D75766
[MLIR] Add `and`, `or`, `xor`, `min`, `max` too gpu.all_reduce and the nvvm lowering ClosedPublic Authored by clementval on Mar 6 2020, 12:07 PM.
Details Summary This patch add some builtin operation for the gpu.all_reduce ops.
This is useful for higher level dialect like OpenACC or OpenMP that can lower to the GPU dialect.
Diff Detail Event TimelineComment Actions I have some tests ready as well but just wanted to check what kind of test are preferred: test running with the mlir-cuda-runner with a check of the result or a mlir-opt lowering with checks of the lowering? Comment Actions
We can't assume that someone has a GPU to be able to run the compiler unit tests: so lit+FileCheck please. Comment Actions Thanks Valentin, the change looks good, with one caveat: the all_reduce lowering in LowerGpuOpsToNVVMOps.cpp is on the chopping block. Comment Actions @csigg Sure I was thinking about it. I'll do it an update the patch. Comment Actions Nice, thanks for adding this!
Comment Actions
It is still nice to have a test with the mlir-cuda-runner to check that it computes what we expect it to compute if you have already written them. But as Mehdi said, testing the generated IR is the mandatory one. Comment Actions
bondhugula added inline comments.
This revision now requires changes to proceed.Mar 10 2020, 5:27 AM Closed by commit rG2eff566b07da: [MLIR] Add `and`, `or`, `xor`, `min`, `max` too gpu.all_reduce and the nvvm… (authored by herhut). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions @herhut Thanks for pushing the patch. I don't see me in the attribution? Is this normal? I don't really mind but just wanted to double check. Comment Actions
Thanks for flagging this. No, this is not normal. It is me not checking that arc patch does the right thing. You should have remained as the author. I can revert and reland with corrected attribution. Comment Actions
Thanks a lot! Really appreciate!
Revision Contents
Diff 249347 mlir/include/mlir/Dialect/GPU/GPUOps.td
mlir/include/mlir/ExecutionEngine/RunnerUtils.h
mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
mlir/lib/Dialect/GPU/Transforms/AllReduceLowering.cpp
mlir/lib/ExecutionEngine/RunnerUtils.cpp
mlir/test/Dialect/GPU/all-reduce-max.mlir
mlir/test/Dialect/GPU/invalid.mlir
mlir/test/mlir-cuda-runner/all-reduce-and.mlir
mlir/test/mlir-cuda-runner/all-reduce-max.mlir
mlir/test/mlir-cuda-runner/all-reduce-min.mlir
mlir/test/mlir-cuda-runner/all-reduce-or.mlir
mlir/test/mlir-cuda-runner/all-reduce-xor.mlir
|
Instead of asserting here, could you extend the verifier to report errors instead? If this combination is illegal, the verifier should reject it.