Page MenuHomePhabricator

[mlir][openacc] Add conversion for if operand to scf.if for standalone data operation
ClosedPublic

Authored by clementval on May 28 2021, 9:00 AM.

Details

Summary

This patch convert the if condition on standalone data operation such as acc.update,
acc.enter_data and acc.exit_data to a scf.if with the operation in the if region.
It removes the operation when the if condition is constant and false. It removes the
the condition if it is contant and true.

Conversion to scf.if is done in order to use the translation to LLVM IR dialect out of the box.
Not sure this is the best approach or we should perform this during the translation from OpenACC
to LLVM IR dialect. Any thoughts welcome.

Diff Detail

Event Timeline

clementval created this revision.May 28 2021, 9:00 AM
clementval requested review of this revision.May 28 2021, 9:00 AM
clementval added a project: Restricted Project.May 28 2021, 9:00 AM

I have three points for discussion,

  1. There is an SCF to OpenMP conversion, I can imagine an SCF to OpenACC conversion as well. This (OpenACC to SCF) would then create a cycle. Is that a problem?
  2. Can this be a transformation pass?
  3. For the OpenMP Parallel operation, the if clause is handled during translation. The CreateParallel function in the OpenMP IRBuilder accepts an if value and inserts an if-then-else. For consistency, should we do this during translation?

https://github.com/llvm/llvm-project/blob/2644399ce7721cba9a546a9af09fd2a942a4d0cd/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L531

  1. There is an SCF to OpenMP conversion, I can imagine an SCF to OpenACC conversion as well. This (OpenACC to SCF) would then create a cycle. Is that a problem?

You are right. I guess it depends how the final pipeline is set up. If the correct patterns are applied at the correct time it should work.

  1. Can this be a transformation pass?

Yeah that's probably better than conversion since it does not really convert the op to another op.

  1. For the OpenMP Parallel operation, the if clause is handled during translation. The CreateParallel function in the OpenMP IRBuilder accepts an if value and inserts an if-then-else. For consistency, should we do this during translation?

https://github.com/llvm/llvm-project/blob/2644399ce7721cba9a546a9af09fd2a942a4d0cd/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L531

I'm fine doing this in translation. The only point I see where it would be more beneficial to pass by SCF is for example if we want to translate to smth else than LLVM IR someday (e.g. SPIR-V). In that case going through scf would allow to use a single transformation to scf.if and reuse the available translation.

  1. Can this be a transformation pass?

@ftynse @rriddle Are there some best practices about conversion passes vs. transformation passes?

clementval updated this revision to Diff 349281.Jun 2 2021, 8:30 AM

Fix header guard

  1. Can this be a transformation pass?

@ftynse @rriddle Are there some best practices about conversion passes vs. transformation passes?

I think this is a good question to discuss in discourse. You can point to this review as well.

  1. Can this be a transformation pass?

@ftynse @rriddle Are there some best practices about conversion passes vs. transformation passes?

I think this is a good question to discuss in discourse. You can point to this review as well.

Sure. Topic opened here; https://llvm.discourse.group/t/conversion-vs-transformation-pass/3567

ftynse added a comment.Jun 4 2021, 3:09 AM

I have three points for discussion,

  1. There is an SCF to OpenMP conversion, I can imagine an SCF to OpenACC conversion as well. This (OpenACC to SCF) would then create a cycle. Is that a problem?

A priori, no. Passes are executed one by one and the pass infrastructure has no knowledge of what happens inside the pass, e.g. whether the pass is using the dialect conversion infrastructure or not so there is no chance of patterns mutually undoing each other effects.

  1. Can this be a transformation pass?

The real reason why lib/Conversion was created is library layering. A pass that converts DialectA to DialectB depends on both dialects for obvious reasons. If we put it in either DialectA or in DialectB, we would create a dependency between dialects that is otherwise avoidable. So we put the pass in lib/Conversion/DialectAToDialectB. If DialectB already depends on DialectA, the pass can live within DialectB just fine because it is not introducing any new dependency.

  1. For the OpenMP Parallel operation, the if clause is handled during translation. The CreateParallel function in the OpenMP IRBuilder accepts an if value and inserts an if-then-else. For consistency, should we do this during translation?

https://github.com/llvm/llvm-project/blob/2644399ce7721cba9a546a9af09fd2a942a4d0cd/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L531

Personally, I would prefer doing as many things as possible in MLIR and have a trivial translation. Most of the complexity currently in the translation infrastructure is only there to support the use of OpenMPIRBuilder. On the other hand, having two pieces of code that serve the same purpose within the larger LLVM infrastructure is also undesirable...

ftynse requested changes to this revision.Jun 4 2021, 3:22 AM
ftynse added inline comments.
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
23

Nit: please add documentation comments for top-level entries

25

Nit: you can put constexpr static kIfConditionOperandPos = 0 in extraClassDeclaration of the relevant op definitions, than use OpTy:kIfConditionOperandPos. This is future-proof should the position change and makes the code a bit more readable.

26–35

Mutating an operation using Operation:: API is invisible to PatternRewriter and may make the entire conversion crash. Wrap this into rewriter.updateRootInPlace().

27–36

Actually, given a concrete op type, it should be possible to do something like op.ifConditionMutable().erase(), which updates the attribute itself without all the magic numbers here.

64–65

This feels like it should be a separate pattern that is also run by the canonicalizer.

69–70

Why is this necessary? The condition has been removed from the op already...

This revision now requires changes to proceed.Jun 4 2021, 3:22 AM
clementval marked an inline comment as done.Jun 4 2021, 12:54 PM
clementval added inline comments.
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
27–36

Thanks! This is what I was looking for. It's way cleaner than the magic numbers stuff and future-proof.

64–65

Move it has a canonicalizer in a separate patch (D103712). Do you think it should it be called here as well?

clementval marked an inline comment as done.

Remove changes that are not needed anymore.

ftynse accepted this revision.Jun 7 2021, 12:46 AM

Please make the rewriter aware of the inplace mutation. Otherwise LGTM.

mlir/include/mlir/Conversion/OpenACCToSCF/ConvertOpenACCToSCF.h
23

I suppose this linter error means you need to #include <memory>

mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
40

This is still an inplace mutation that needs rewriter.updateRootInPlace around it.

64–65

You can add the canonicalization pattern here as well if it help enable other patterns.

This revision is now accepted and ready to land.Jun 7 2021, 12:46 AM
clementval updated this revision to Diff 350296.Jun 7 2021, 8:07 AM
clementval marked 2 inline comments as done.

Address review comments

ftynse added inline comments.Jun 7 2021, 8:09 AM
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
41

Sorry, missed this one before, getThenBodyBuilder is unaware of the rewriter either. You need to either set the rewriter to the same location or call .setListener(rewriter.getListener()) on it before using.

clementval updated this revision to Diff 350310.Jun 7 2021, 8:33 AM

Address review comments.

clementval marked an inline comment as done.Jun 7 2021, 8:33 AM
clementval added inline comments.
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
41

Thanks. Didn't know we had to do this here.

ftynse added inline comments.Jun 7 2021, 8:43 AM
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
41

Generally, anything IR modification needs to go through the rewriter if you have one. So any call that is not rewriter. needs to be communicated to the rewriter somehow.

clementval marked an inline comment as done.Jun 7 2021, 8:50 AM
clementval added inline comments.
mlir/lib/Conversion/OpenACCToSCF/OpenACCToSCF.cpp
41

Got it. Thanks!

This revision was landed with ongoing or failed builds.Jun 7 2021, 9:10 AM
This revision was automatically updated to reflect the committed changes.