Skip to content

[MLIR][OpenMP] Document entry block argument-defining clauses (NFC) #109811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

skatrak
Copy link
Member

@skatrak skatrak commented Sep 24, 2024

This patch adds general information on the proposed approach to unify the handling and representation of clauses that define entry block arguments attached to operations that accept them.

@llvmbot
Copy link
Member

llvmbot commented Sep 24, 2024

@llvm/pr-subscribers-mlir-openmp

@llvm/pr-subscribers-mlir

Author: Sergio Afonso (skatrak)

Changes

This patch adds general information on the proposed approach to unify the handling and representation of clauses that define entry block arguments attached to operations that accept them.


Full diff: https://github.com/llvm/llvm-project/pull/109811.diff

1 Files Affected:

  • (modified) mlir/docs/Dialects/OpenMPDialect/_index.md (+69-1)
diff --git a/mlir/docs/Dialects/OpenMPDialect/_index.md b/mlir/docs/Dialects/OpenMPDialect/_index.md
index 88437b8cf828cc..3c30b29d09356b 100644
--- a/mlir/docs/Dialects/OpenMPDialect/_index.md
+++ b/mlir/docs/Dialects/OpenMPDialect/_index.md
@@ -285,7 +285,75 @@ argument's type:
   specific `mlir::Attribute` subclass) will be used instead.
   - Other attribute types will be represented with their `storageType`.
 - It will create `<Name>Operands` structure for each operation, which is an
-empty structure subclassing all operand structures defined for the corresponding `OpenMP_Op`'s clauses.
+empty structure subclassing all operand structures defined for the corresponding
+`OpenMP_Op`'s clauses.
+
+### Entry Block Argument-Defining Clauses
+
+Certain OpenMP clauses introduce in their MLIR representation mappings between
+outside values and entry block arguments for the region of the MLIR operation
+they are applied to. This enables, for example, the introduction of private
+copies of the same underlying variable. Currently, clauses with this property
+can be classified in three main categories:
+  - Map-like clauses: `map`, `use_device_addr` and `use_device_ptr`.
+  - Reduction-like clauses: `in_reduction`, `reduction` and `task_reduction`.
+  - Privatization clause: `private`.
+
+All three kinds of entry block argument-defining clauses use a similar custom
+assembly format representation, only differing based on the different pieces of
+information attached to each kind. Below, one example of each is shown:
+
+```mlir
+omp.target map_entries(%x -> %x.m, %y -> %y.m : !llvm.ptr, !llvm.ptr) {
+  // Use %x.m, %y.m in place of %x and %y...
+}
+
+omp.wsloop reduction(@add.i32 %x -> %x.r, byref @add.f32 %y -> %y.r : !llvm.ptr, !llvm.ptr) {
+  // Use %x.r, %y.r in place of %x and %y...
+}
+
+omp.parallel private(@x.privatizer %x -> %x.p, @y.privatizer %y -> %y.p : !llvm.ptr, !llvm.ptr) {
+  // Use %x.p, %y.p in place of %x and %y...
+}
+```
+
+As a consequence of parsing and printing the operation's first region entry
+block argument names together with the custom assembly format of these clauses,
+entry block arguments (i.e. the `^bb0(...):` line) must not be explicitly
+defined for these operations. Additionally, it is not possible to implement this
+feature while allowing each clause to be independently parsed and printed,
+because they need to be printed/parsed together with the corresponding
+operation's first region. They must have a well-defined ordering in which
+multiple of these clauses are specified for a given operation, as well.
+
+The parsing/printing of these clauses together with the region provides the
+ability to define entry block arguments directly after the `->`. Forcing a
+specific ordering between these clauses makes the block argument ordering
+well-defined, which is the property used to easily match each clause with the
+entry block arguments defined by it.
+
+Custom printers and parsers for operation regions based on the entry block
+argument-defining clauses they take are implemented based on the
+`{parse,print}BlockArgRegion` functions, which take care of the sorting and
+formatting of each kind of clause, minimizing code duplication resulting from
+this approach. One example of the custom assembly format of an operation taking
+the `private` and `reduction` clauses is the following:
+
+```tablegen
+let assemblyFormat = clausesAssemblyFormat # [{
+  custom<PrivateReductionRegion>($region, $private_vars, type($private_vars),
+      $private_syms, $reduction_vars, type($reduction_vars), $reduction_byref,
+      $reduction_syms) attr-dict
+}];
+```
+
+The `BlockArgOpenMPOpInterface` has been introduced to simplify the addition and
+handling of these kinds of clauses. It holds `num<ClauseName>BlockArgs()`
+functions that by default return 0, to be overriden by each clause through the
+`extraClassDeclaration` property. Based on these functions and the expected
+alphabetical sorting between entry block argument-defining clauses, it
+implements `get<ClauseName>BlockArgs()` functions that are the intended method
+of accessing clause-defined block arguments.
 
 ## Loop-Associated Directives
 

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@bhandarkar-pranav bhandarkar-pranav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Given how reviewing docs essentially turns into a series of subjective opinions or preferences, please consider most all of my comments as nits, except the one about alphabetical sorting of clauses.

handling of these kinds of clauses. It holds `num<ClauseName>BlockArgs()`
functions that by default return 0, to be overriden by each clause through the
`extraClassDeclaration` property. Based on these functions and the expected
alphabetical sorting between entry block argument-defining clauses, it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming the tablegen backend for openmp that you have implemented doesn't do the sorting and the onus for the alphabetical sorting is on the user, correct? If that's the case i think that expectation must be made explicit, either here or (preferably) in Adding an Operation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality the alphabetical sorting of entry block argument-defining clauses is enforced by the custom parsers/printers and the interface itself, regardless of the order in which these clauses are specified in the operation's definition by the user. Either way, I think it's a good idea to state in the section you mention that the convention is to specify clauses in alphabetical order.

Copy link
Member Author

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback Pranav, let me know if the changes address your concerns.

handling of these kinds of clauses. It holds `num<ClauseName>BlockArgs()`
functions that by default return 0, to be overriden by each clause through the
`extraClassDeclaration` property. Based on these functions and the expected
alphabetical sorting between entry block argument-defining clauses, it
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In reality the alphabetical sorting of entry block argument-defining clauses is enforced by the custom parsers/printers and the interface itself, regardless of the order in which these clauses are specified in the operation's definition by the user. Either way, I think it's a good idea to state in the section you mention that the convention is to specify clauses in alphabetical order.

@bhandarkar-pranav
Copy link
Contributor

Thanks for the feedback Pranav, let me know if the changes address your concerns.

Thank you for the changes. This LGTM.

Copy link
Contributor

@bhandarkar-pranav bhandarkar-pranav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@skatrak skatrak force-pushed the users/skatrak/block-args-03-usedevice branch from f61e3a6 to c08a30e Compare October 1, 2024 15:01
@skatrak skatrak force-pushed the users/skatrak/block-args-04-docs branch from 3cb071d to 32735e3 Compare October 1, 2024 15:02
@skatrak skatrak force-pushed the users/skatrak/block-args-03-usedevice branch from c08a30e to 56649e8 Compare October 1, 2024 15:45
Base automatically changed from users/skatrak/block-args-03-usedevice to main October 1, 2024 15:46
This patch adds general information on the proposed approach to unify the
handling and representation of clauses that define entry block arguments
attached to operations that accept them.
@skatrak skatrak force-pushed the users/skatrak/block-args-04-docs branch from 32735e3 to ce1c270 Compare October 1, 2024 15:47
@skatrak skatrak merged commit 4e52e6a into main Oct 1, 2024
4 of 5 checks passed
@skatrak skatrak deleted the users/skatrak/block-args-04-docs branch October 1, 2024 15:48
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
…lvm#109811)

This patch adds general information on the proposed approach to unify
the handling and representation of clauses that define entry block
arguments attached to operations that accept them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants