[PATCH] D78447: [mlir][SCCP] Add support for propagating constants in the presence of region based control flow.

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

In D78447#1991521 <https://reviews.llvm.org/D78447#1991521>, @rriddle wrote:

> In D78447#1991402 <https://reviews.llvm.org/D78447#1991402>, @bondhugula wrote:
>
> > > may be executed when entering an operation, and which regions are executed by "branching" from other regions
> >
> > This could be confusing since one can't directly branch from another region (as in using a 'br'). Change "by branching from other regions" to "after having executed another region of the parent op". Also, this commit summary has more information than the ODS description of getSuccessorRegions in some ways. Consider making the ODS doc desc. more comprehensive.
> >
> > > The operands may correspond to a subset of the entry arguments to the region.
> >
> > I think you meant the other way? The entry arguments to the region may correspond to a subset of the operands.
>
>
> No, I mean the operands returned by this method may correspond to a subset of the entry arguments. This method shouldn't return more operands than there are entry block arguments.


Oh, I was confused thinking about the parent op's operands. Consider changing:
"The operands may correspond to a subset of the entry arguments to the region." -> ".... entry arguments of the containing region"



================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:179
+    /// `getSuccessorRegions`.
+    OperandRange getRegionEntryOperands(unsigned index);
   }];
----------------
I think this should be called `getSuccessorRegionEntryOperands` or `getSuccRegionEntryOperands`.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:179
+    /// `getSuccessorRegions`.
+    OperandRange getRegionEntryOperands(unsigned index);
   }];
----------------
bondhugula wrote:
> I think this should be called `getSuccessorRegionEntryOperands` or `getSuccRegionEntryOperands`.
Since this op has exactly one region and so `index` can only be zero, update the doc comment to reflect that.


================
Comment at: mlir/include/mlir/Interfaces/ControlFlowInterfaces.h:74
+private:
+  Region *successor;
+  ValueRange successorInputs;
----------------
`Region *region` or else it gives the impression that this is a successor to that successor region. 


================
Comment at: mlir/include/mlir/Interfaces/ControlFlowInterfaces.h:75
+  Region *successor;
+  ValueRange successorInputs;
+};
----------------
`successorInputs` -> `inputs` likewise. This is already a `RegionSuccessor` class.


================
Comment at: mlir/include/mlir/Interfaces/ControlFlowInterfaces.td:105
+    InterfaceMethod<[{
+        Return the operands used for the region at `index`, which was specified
+        as a successor by `getSuccessorRegions`. when entering. These operands
----------------
Spell out "used" here, i.e., "used as arguments" or "used as the entry block arguments". 


================
Comment at: mlir/include/mlir/Interfaces/ControlFlowInterfaces.td:107
+        as a successor by `getSuccessorRegions`. when entering. These operands
+        should correspond 1-1 with the successor inputs specifed in
+        `getSuccessorRegions`.
----------------
typo: specified


================
Comment at: mlir/include/mlir/Interfaces/ControlFlowInterfaces.td:121
+        `index` is None, `operands` is a set of optional attributes that
+        correspond to a constant value for each operand of this operation, or
+        null if that operand is not a constant. If `index` is valid, `operands`
----------------
Nit: that either correspond


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:343
+  // The `then` and the `else` region branch back to the parent operation.
+  if (index) {
+    regions.push_back(RegionSuccessor(getResults()));
----------------
index.hasValue() to avoid confusion. What if index has a value and is zero? I can't recall.


================
Comment at: mlir/lib/Transforms/SCCP.cpp:526-528
+    // If this terminator is not "region-like", conservatively mark all of the
+    // successor values as overdefined.
+    if (!op->hasTrait<OpTrait::ReturnLike>()) {
----------------
rriddle wrote:
> rriddle wrote:
> > bondhugula wrote:
> > > You missed marking `ReturnOp` `ReturnLike`.  As such, you will return early here and miss propagating lattice states to successor regions. Although there is no registered op that will exercise this, this could happen let's say in a special func-like op that has two regions (sort of two functions) and the semantics of the op were to execute the two regions one after the other. 
> > Such an op isn't possible ATM given the constraints of ReturnOp. I intend to add interprocedural support here, at which point having it on ReturnOp would be useful. Also as a policy I like to avoid trying to mark every possible op that could take advantage of something. IMO it is better to separate adding a feature, and using it in every possible place. It keeps this patch more focused.
> To clarify, I do intend to add this trait/interface to the things that can use it in a followup.
It's just weird that the standard ReturnOp isn't marked ReturnLike. You could add the support to exploit it later, but no harm marking the more prevalent / core one? Yes, there isn't a need to add it in every other possible place now. OTOH, if ReturnLike doesn't work well for the ReturnOp and you need another trait, then this one should be renamed YieldLike and ReturnLike could be used for ReturnOp and for other declarative ops.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78447/new/

https://reviews.llvm.org/D78447