[PATCH] D80358: [MLIR][InProgress] Add RegionKindInterface
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 16 01:04:25 PDT 2020
rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.
Made a first pass through the docs. Thank you for cleaning this up!
================
Comment at: mlir/docs/Interfaces.md:209
+
+MLIR provides a suite of interface that provide various functionalities that are
+common across many different operations. Below is a list of some key traits that
----------------
mehdi_amini wrote:
> interfaces? (plural)
> provides? (`a suite` is the subject I believe)
nit: suite of interfaces
================
Comment at: mlir/docs/Interfaces.md:210
+MLIR provides a suite of interface that provide various functionalities that are
+common across many different operations. Below is a list of some key traits that
+may be used directly by any dialect. The format of the header for each trait
----------------
nit: key interfaces
================
Comment at: mlir/docs/LangRef.md:30
+called [Operation](#operations), and edges, called *Values*. Each
+Value is defined as the result of exactly one Operation, and has a
+*Value Type* defined by the [type system](#type-system). Operations
----------------
nit: I find this definition confusing given that arguments to blocks are also values.
================
Comment at: mlir/docs/LangRef.md:204
-The scope of SSA values is defined based on the standard definition of
-[dominance](https://en.wikipedia.org/wiki/Dominator_\(graph_theory\)). Argument
-identifiers in mapping functions are in scope for the mapping body. Function
-identifiers and mapping identifiers are visible across the entire module.
+Value identifiers are in scope for the region in which they are
+defined. Argument identifiers in mapping functions are in scope for
----------------
nit: are only in scope
================
Comment at: mlir/docs/LangRef.md:206
+defined. Argument identifiers in mapping functions are in scope for
+the mapping body. Function identifiers and mapping identifiers are
+visible across the entire module. Particular operations may further
----------------
This isn't true. This goes into symbol table semantics, which scopes the visibility with its own set of rules. https://mlir.llvm.org/docs/SymbolsAndSymbolTables/
================
Comment at: mlir/docs/LangRef.md:420
+Blocks in MLIR take a list of block arguments, notated in a
+function-like way. Block arguments are bound to values produced by
+all terminator operations, i.e. Branches, which connect to the
----------------
Can you reword this? It gives off the vibe that the terminator is actually generating the value, but terminator operations don't generally have results. Some terminator operations may produce the PHI value internally, but those are not very common. I would rephrase to "provided" or something similar.
================
Comment at: mlir/docs/LangRef.md:497
+any. By default, a region can reference values defined outside of the
+region whenever it would have been legal to use them as operands to
+the enclosing operation.
----------------
nit: Can you mention IsolatedFromAbove here and link to the doc on it?
================
Comment at: mlir/docs/LangRef.md:535
+standard notion of sequential control flow through basic
+blocks. However, the particular semantics of operations are
+unspecified and completely specify the behavior of an operation and
----------------
> However, the particular semantics of operations are
> unspecified and completely specify the behavior of an operation and
> inside the regions of an operation.
Can you rephrase this? I'm having trouble parsing it.
================
Comment at: mlir/docs/LangRef.md:538
+inside the regions of an operation. In MLIR, control flow semantics of
+a region is indicated by `RegionKindInterface::SSACFG`.
+
----------------
nit: Link to the documentation for this.
================
Comment at: mlir/docs/LangRef.md:545
+Terminator operations ending each block represent control flow by
+explicitly specifying the successor blocks of the block. Control flow
+can only pass to one of the specified successor blocks as in a
----------------
nit: the potential successor blocks
================
Comment at: mlir/docs/LangRef.md:553
+entry block which are not listed as a successor of a terminator
+operation are unreachable and can be removed without affecting the
+semantics of the containing operation.
----------------
nit: are treated as unreachable and may be
================
Comment at: mlir/docs/LangRef.md:577
+control flow to multiple contained regions concurrently. An operation
+may also pass control flow into Regions that were specified in other
+operations, in particular those that defined the values or symbols the
----------------
nit: Regions -> regions
================
Comment at: mlir/docs/LangRef.md:41
+target-specific instructions, configuration registers, and logic
+gates. These different concepts are represented by *Operation Types*,
+which can be arbitrarily extended in MLIR. Verification constraints
----------------
mehdi_amini wrote:
> stephenneuendorffer wrote:
> > mehdi_amini wrote:
> > > The paragraph starts with "Operations can represent many different concepts", and now the sentence says "These different concepts are represented by *Operation Types*" ; this does not fit.
> > >
> > > Maybe you're using "Operation Types" to define what is just "Operation" in MLIR? Since "Type" is already an IR concept I'd avoid this here.
> > >
> > I was trying to get at at the distinction between class Operation and class Op. To me "Operation Type" is distinct from "Data Types processed by an Operation". I scratched my head for better wording here, but felt like the other things I came up with didn't work either. Suggestions welcome here if you have a better way to describe this?
> What about something like: `These different concepts are expressed by attaching to an Operation definition: traits, interfaces, and more importantly verification constraints`.
I feel the same way as Mehdi. I would rephrase to something like:
```
Operations can be arbitrarily extended in MLIR, and may represent many different concepts ...
...
```
I would even consider removing the bit about verification. Feels a bit out of place here.
================
Comment at: mlir/docs/LangRef.md:539
+For dialects associated with 'control flow' semantics, MLIR does not
+restrict how control flow enters or exits a region, or how it flows
+between the blocks in a region. Terminator operations typically
----------------
mehdi_amini wrote:
> stephenneuendorffer wrote:
> > mehdi_amini wrote:
> > > I thought we were going with CFG semantics for non single-block regions? Did you change your mind here?
> > >
> > > That implies that we mandate the first block being the entry block.
> > >
> > I think the discussion was about how to handle unregistered dialects. In thinking about this further it seems unintuitive to me that some code in a basic block would be legal, but become illegal when an additional basic block is added. E.g.:
> >
> > Legal.. Might be a 'graph' operation:
> > "unregistered_op" () ({
> > ^bb1:
> > %2 = %1
> > %1 = %2
> > }
> >
> > Should be legal? Might be a graph-like operator using blocks to model something.
> > "unregistered_op" () ({
> > ^bb1:
> > %2 = %1
> > %1 = %2
> > ^bb2:
> > %3 = %2
> > }
> >
> > It seems more consistent to me to just let the second case be legal. Once the dialect is registered, I think it is reasonable to require a dialect to specify whether either case is legal or not.
> by doing this you're introducing a mix of "graph-like" and CFG, which seems iffy to me right now.
>
I don't agree on the principal here. We only have to be conservative if it "could" be something that has valid semantics when registered. For example, we have to be conservative about unregistered operations that "could" be symbol tables, but that doesn't mean we error out if we see any unregistered region holding operation. Being unregistered IMO is just having the capability of being anything legal. If something isn't legal when registered, why should we guard against it when unregistered?
================
Comment at: mlir/docs/LangRef.md:546
+control to other basic blocks is determined by the specific dialect
+operations involved.
+
----------------
mehdi_amini wrote:
> stephenneuendorffer wrote:
> > mehdi_amini wrote:
> > > This is missing the "reachability" aspect: the entry block is only for entry, and any other block is only reachable if it is in the successor list of another operation in the region.
> > Hmm... So terminators are required to pass control flow to their successors? A terminator without successors is not allowed to pass control flow to an arbitrary block in the same region?
> That's how we decide "unreachable blocks" today
Correct. There is no modeling ATM for indirect branches where you have no idea what the potential destination could be.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:1
+//===- RegionKindInterface.h - Infer Type Interfaces -----------*- C++ -*-===//
+//
----------------
This line looks like it is one character to short.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:14
+
+#ifndef MLIR_REGIONKINDINTERFACE_H_
+#define MLIR_REGIONKINDINTERFACE_H_
----------------
Please fix the header guard style.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:17
+
+#include "mlir/IR/Attributes.h"
+#include "mlir/IR/Builders.h"
----------------
Can you trim these?
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:26
+
+enum class RegionKind {
+ Unknown,
----------------
Please add documentation here on what these are and what they mean.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.h:29
+ SSACFG,
+ GRAPH,
+};
----------------
Graph? SSACFG contains acronyms so I see why they are capitalized.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:27
+ StaticInterfaceMethod<
+ /*desc=*/[{Return the types of regions in an operation
+
----------------
nit: Can you reflow this and move it to the next line? The doc formatter can handle this automatically.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:27
+ StaticInterfaceMethod<
+ /*desc=*/[{Return the types of regions in an operation
+
----------------
rriddle wrote:
> nit: Can you reflow this and move it to the next line? The doc formatter can handle this automatically.
Missing period at the end of the summary.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:30
+ By default, operations of registered dialects contain regions with CFG
+ semantics. This implies that they must satisfy the SSA dominance property.
+ However, other kinds of regions are often used which may not require SSA
----------------
nit: Remove double space.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:39
+ StaticInterfaceMethod<
+ /*desc=*/"Return true if the kind of the given region requires the "
+ "SSA-Dominance property",
----------------
nit: Please use multi line string for this description.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:47
+ }]
+ // /*defaultImplementation=*/[{
+ // /// Returns whether two arrays are equal as strongest check for
----------------
Leftover debugging?
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:55
+
+ // let verify = [{
+ // return detail::verifyInferredResultTypes($_op);
----------------
Please remove.
================
Comment at: mlir/include/mlir/IR/RegionKindInterface.td:60
+
+#endif // MLIR_REGIONKINDINTERFACE
----------------
Please fix the header guard style.
================
Comment at: mlir/include/mlir/Interfaces/CMakeLists.txt:7
add_mlir_interface(SideEffectInterfaces)
add_mlir_interface(ViewLikeInterface)
----------------
Seems unrelated.
================
Comment at: mlir/lib/IR/RegionKindInterface.cpp:18
+
+namespace mlir {
+#include "mlir/IR/RegionKindInterface.cpp.inc"
----------------
I don't think this is necessary.
================
Comment at: mlir/lib/IR/Verifier.cpp:31
#include "mlir/IR/Dominance.h"
+#include "mlir/IR/OpDefinition.h"
#include "mlir/IR/Operation.h"
----------------
I don't think this is necessary.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80358/new/
https://reviews.llvm.org/D80358
More information about the llvm-commits
mailing list