[PATCH] D78447: [mlir][SCCP] Add support for propagating constants across inter-region control flow

Mon Apr 20 10:48:27 PDT 2020

rriddle added a comment.

Thanks for the review Uday! I appreciate the attention to detail.

================
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:
> 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.
Agreed, went ahead and added it. Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78447