[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 requested changes to this revision.
bondhugula added a comment.
This revision now requires changes to proceed.

> 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.



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78447