Page MenuHomePhabricator

[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.

  • 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.

Diff Detail

Event Timeline

clementval created this revision.Mar 6 2020, 12:07 PM
clementval added a comment.EditedMar 6 2020, 12:09 PM

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?

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 miler-opt lowering with checks of the lowering?

We can't assume that someone has a GPU to be able to run the compiler unit tests: so lit+FileCheck please.

csigg added a comment.Mar 7 2020, 10:03 AM

Thanks Valentin, the change looks good, with one caveat: the all_reduce lowering in LowerGpuOpsToNVVMOps.cpp is on the chopping block.
Would you mind replicating your changes in mlir/lib/Dialect/GPU/Transforms/AllReduceLowering.cpp as well?
I hope I will get to the clean up next week, it's overdue but I've been busy with other stuff.

@csigg Sure I was thinking about it. I'll do it an update the patch.
@mehdi_amini Thanks for the confirmation. I'll add some tests to this patch then.

clementval edited the summary of this revision. (Show Details)Mar 7 2020, 11:12 AM
herhut added a comment.Mar 9 2020, 2:58 AM

Nice, thanks for adding this!

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
127

Instead of asserting here, could you extend the verifier to report errors instead? If this combination is illegal, the verifier should reject it.

herhut added a comment.Mar 9 2020, 3:01 AM

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 miler-opt lowering with checks of the lowering?

We can't assume that someone has a GPU to be able to run the compiler unit tests: so lit+FileCheck please.

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.

jdoerfert resigned from this revision.Mar 9 2020, 8:35 AM
clementval updated this revision to Diff 249195.Mar 9 2020, 1:02 PM
  • Add some mlir-cuda-runner tests
  • Add some mlir-opt + FileCheck tests
  • Address other comments

@csigg @herhut I updated the patch with your suggestions.

bondhugula requested changes to this revision.Mar 10 2020, 5:27 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
156

Nit: "" -> ''

mlir/lib/ExecutionEngine/RunnerUtils.cpp
46

Nit: M->rank is int64_t.

46

Unrelated to this patch: an UnrankedMemRefType having a ->rank field is weird. Would have been better to name it UnknownRankMemRefType FWIW.

77

List initialization?
UnrankedMemRefType<int32_t> descriptor = {rank, ptr};

This revision now requires changes to proceed.Mar 10 2020, 5:27 AM
clementval marked an inline comment as done.

Address @bondhugula comments

clementval marked 3 inline comments as done.Mar 10 2020, 6:02 AM
herhut accepted this revision.Mar 10 2020, 7:36 AM

Thanks for addressing the comments. Looks good to land.

@herhut Thanks for the review. Do you mind pushing it? I do not have access rights.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 10 2020, 1:41 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 1:41 PM

@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.

@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.

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.

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.

Thanks a lot! Really appreciate!