[PATCH] D75837: Introduce std.execute_region op

[Sean Silva via Phabricator via llvm-commits]

silvas added a comment.

> +1 This is also exactly what I wanted to say. If there were arguments in the land you were starting from (say you were inlining a call), those arguments should just get propagated and eliminated. Keeping arguments around will necessitate all kinds of tracking/bookkeeping in moving code across, reimplementing existing canonicalizations on this op and largely defeating the purpose of this op - which is to let SSA dominance and dataflow work freely from above and through it.

Since river is working on making dataflow be able to transparently look through non-explicit captures for ops like this, how important is it to not have explicit args?

That is, we can canonicalize the args away, but having the args shouldn't hurt? If anything, allowing the removal of trivial args where it makes sense be a canonicalization on the execute_region op avoids pushing the responsibility on clients producing the op to create the op in that form initially. E.g. when inlining a calls as in the initial use case, you would just throw the FuncOp's region as-is into an execute_region op (updating "return" terminators perhaps), and transfer over the arg list of the call to the execute_regoin op. Otherwise, they would have to do the arg rewriting themselves (maybe we can just have a helper function for that though).

I guess I'm trying to understand whether we expect code to see code like this:

  if (auto executeRegion = dyn_cast<ExecuteRegionOp>(op)) {
    if (executeRegion.hasExplicitCaptures()) {
      break; // Darn, can't handle it.
    }
  }

I expect that we won't have code like this, and instead what we'll see is generic use-def following passes that silently aren't smart enough to handle explicit captures (such as when applying local rewrite patterns) and will fail to optimize. So I think the real question is balancing:

1. The cost of pushing all clients of this op to establish the canonical form of no explicit captures mandatorily upon creation
2. The potential lost optimization opportunities due failing (for whatever reason; oversight, pass ordering issues, ...) to run the canonicalization pass to put it into the no-explicit-capture form.

Neither seems massively compelling, so starting with the more restricted form seems like a good choice. We can loosen it later if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75837