[PATCH] D78863: [MLIR] Introduce op trait PolyhedralScope

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula added a comment.

In D78863#2007580 <https://reviews.llvm.org/D78863#2007580>, @ftynse wrote:

> In D78863#2007528 <https://reviews.llvm.org/D78863#2007528>, @gribozavr2 wrote:
>
> > Sorry, I reverted this change in https://github.com/llvm/llvm-project/commit/ef06016d73390d5b380018cc0d16003b4ed4a35a. @ftynse understands the problem and will respond here a bit later.
>
>
> This broke IREE and also was problematic with Bazel builds. There are two issues, and I posit that one of them caused another.
>
> 1. IREE pass pipeline triggered `llvm_unreachable("op doesn't have an enclosing polyhedral scope");` (AffineOps.cpp:119). We investigated this and found that this pipeline _first_ transforms `std.func` into `gpu.func`, and then runs transformations that depend transitively on Affine within `gpu.func`. The latter not having the `PolyhedralScope` trait, and with no other function like op around, the loop in `getAffineScope` completed and the assertion was triggered. An obvious solution to this is to give `gpu.func` op the trait. However, I think this failure rather indicates a deeper problem with the affine scope modeling: nothing in the semantics of the Affine Ops requires (and verifies) that they _only_ appear within some op with a PolyhedralScope. So the control flow path with "llvm_unrechable" actually _is_ reachable by valid IR. We can either add such requirement, or consider top-level ModuleOp (which should be there by default) to also be a polyhedral scope. This needs further discussion.


As you hint, marking the gpu.func with PolyhedralScope isn't really a solution, although we may want to do that in the future. Marking the `ModuleOp` PolyhedralScope appears to be the right fix to me - the root op is always a valid polyhedral scope.

> 2. The introduction of `Affine/Traits.h` created a circular dependency between "IR" and "AffineDialect" libraries (AffineDialect depends on IR because it uses core IR constructs, and IR depends on AffineDialect because `Function` has a trait defined in Affine). It is actually possible to circumvent the issue in Bazel build files we use, e.g., for TensorFlow, but it is considered to be a hack. Since this specific issue was discussed in the review, let's iterate one more time on it. It party stems from the original lack of separation between core IR and Affine+Standard dialects. Factoring `Function` out of the "IR" library can be one way to break the cycle. However, the issue of dependency on the affine dialect only for the purpose of having a trait remains. Generally, any dialect with region-carrying ops that wishes to contain ops from the Affine dialect has to depend on it, e.g. GPU. This sounds wrong layering-wise. Since we don't want dialect-specific traits to live in IR, another possible separation is to have dialect-specific traits live in a library (and filepath) separate from both IR and Dialect, with both IR and Dialect depending on this library.

This was due to oversight. It was River's suggestion to include Affine/Traits.h from `Function`, but he perhaps intended for it to be in IR/ - I earlier had it in OpDefinition.h where it didn't create a cyclic dependence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78863