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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

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.



================
Comment at: mlir/include/mlir/Interfaces/ControlFlowInterfaces.h:93-94
+/// contain successors.
+template <typename ConcreteType>
+struct ReturnLike : public TraitBase<ConcreteType, ReturnLike> {
+  static LogicalResult verifyTrait(Operation *op) {
----------------
bondhugula wrote:
> Is this the right file to put this trait? Ops that don't implement the op interface above should have access to this trait? (Plus, see the comment on ReturnOp below.)
Yes this is the right place. OpDefinition.h is not the right place to put every possible trait, and is something that I've been purposely moving away from. It is better to keep the interfaces and traits that pertain to a specific abstraction in one place, e.g., the traits *and* interfaces for side effect modeling are in SideEffects.h. Realistically the files in this directory should remove the redundant "Interfaces" from the end of the name.


================
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>()) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78447