This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add test for multiple gpu.all_reduce in the same kernel when lowering to NVVM
ClosedPublic

Authored by clementval on Mar 10 2020, 9:19 AM.

Details

Summary

This patch add tests when lowering multiple gpu.all_reduce operations in the same kernel. This was previously failing.

Diff Detail

Event Timeline

clementval created this revision.Mar 10 2020, 9:19 AM
herhut requested changes to this revision.Mar 10 2020, 10:30 AM
herhut added inline comments.
mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
386 ↗(On Diff #249415)

It would be better to use SymbolTable::insert for this. See KernelOutlining.cpp for an example.

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

@herhut Thanks for the pointer to SymbolTable::insert. I have updated the patch accordingly.

csigg@ just landed some code that does in-dialect lowering and should achieve the same that is done here via using function attributions, instead. Can you verify?

clementval retitled this revision from [MLIR] Allows multiple gpu.all_reduce in the same kernel when lowering to NVVM to [MLIR] Add test for multiple gpu.all_reduce in the same kernel when lowering to NVVM.

csigg@ just landed some code that does in-dialect lowering and should achieve the same that is done here via using function attributions, instead. Can you verify?

The patch from @csigg works fine. I updated the patch with just the tests if it makes sense to add them. Otherwise just discard the patch.

herhut requested changes to this revision.Mar 12 2020, 4:25 AM

Thanks for the extra tests, more tests are always better. Could you fix the Summary in the commit message and I will land it.

mlir/test/Dialect/GPU/multiple-all-reduce.mlir
22

We should not rely on actual names here, as those might change. Does this test currently pass? In any case, please use @{{.*}} instead.

This revision now requires changes to proceed.Mar 12 2020, 4:25 AM
clementval edited the summary of this revision. (Show Details)
clementval marked an inline comment as done.

@herhut I updated the test. It was passing but for sure if the name of the buffer were to change it would fail as you mentioned. By the way I heard from Medhi that if I used arc the attribution would be done correctly for the patch. Will use it for my next patches.

mravishankar resigned from this revision.Mar 17 2020, 12:46 PM
ftynse accepted this revision.Mar 18 2020, 2:37 AM

@ftynse Thanks for the review. As I do not have commit right, do you mind pushing this patch? I did not create it with arc so attribution my not be done correctly.

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