[PATCH] D75837: Introduce std.execute_region op

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added a comment.

Sure, the traditional, run-of-the-mill properties are true:

- implicit captures preserve use-def chains
- arguments break use-def chains and if you want similar optimizations you'll want inlining or some form of IPO.

I see the discussion above as conflating semantics with optimization.
Allowing your op to take argument does **absolutely not mean you have to use them for everything all the time**, yet the argumentation seems to take that as a premisse.
I think this is particularly clear in the following:

>   You'd have to reimplement nearly all canonicalizations on this op from propagation of constants, to propagation of memref_casts, removal of dead deallocs, removal of dead allocs, subexpression elimination, etc.. 

I don't see how this is true and why you'd have to reimplement anything in this list.
If you want these canonicalizations to apply immediately, you should just use implicit capture for all values (which is what you propose).

You could also want to use arguments for a subset of the values, isolate their users, inline and then apply the remaining canonicalizations.
That would be perfectly fine too.

I still see no compelling reason to strictly forbid arguments in this op: if you want to enforce somewhere that everything is by-capture only, it's easy to verify that `numArguments == 0`.
OTOH adding arguments is trivial and will be transparent wrt everything you mention above: if you don't want to use arguments just don't use argument.
Literally, if you added the possibility for your op to have arguments, your canonicalization test would not change.

Plainly forbidding arguments has a finality to it that I view as unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75837