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

Uday Bondhugula via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 27 21:01:24 PDT 2020


bondhugula added inline comments.


================
Comment at: mlir/include/mlir/IR/OpDefinition.h:1056
+/// operation. For more details, see `Traits.md#PolyhedralScope`.
+template <typename ConcreteType>
+class PolyhedralScope : public TraitBase<ConcreteType, PolyhedralScope> {
----------------
rriddle wrote:
> bondhugula wrote:
> > rriddle wrote:
> > > bondhugula wrote:
> > > > rriddle wrote:
> > > > > Is this trait applicable to anything outside of the affine dialect? If not I don't really see why it should be here and not in the affine dialect instead.
> > > > I was in two minds on this. Could you see the second para of the commit summary? We'd like to mark FuncOp with PolyhedralScope. 
> > > Ah, sorry I missed that. A lot of times I skip the commit message and jump right in. 
> > > 
> > > Yes, marking FuncOp makes things a bit difficult. This has come up before w.r.t interfaces as well. I'd like to avoid adding special dialect traits here if they aren't general(especially because I'd like to trim this file a bit). I would expect that you could also use FunctionLike(instead of FuncOp) as an indicator of a polyhedral scope. I'm leaning more towards moving the trait to Affine unless you can foresee uses outside of that dialect.
> > There could  be things that don't depend on the Affine dialect but would want to use the 'PolyhedralScope' on their FuncLike or other region ops. That's because their lowering could potentially go through the affine dialect much later which their region holding op lasts through - is this meaningful?
> > 
> > But when I initially started with keeping this in the Affine dialect, all the clean and compact code that we have now becomes weird. To be precise:
> > 
> > `op->hasTrait<OpTrait::PolyhedralScope>` becomes
> > `op->hasTrait<OpTrait::PolyhedralScope> || isa<FuncOp>(op)`
> > 
> > We can't right away use FuncLikeOp (although we want to go in that direction) because not all func op's may want to opt into this; you'd want to whitelist the desired ones by attaching the PolyhedralScope trait - they may or may not depend on the affine dialect.  Not having `PolyhedralScope` attached to `FuncOp` would mean a  simple `getParentWithTrait<PolyhedralScope>` becomes an entire while loop that tries to find an op that either has the PolyhedralScope trait or is a FuncOp.
> > 
> I agree that it is easier to manipulate when there is one interface for detection. Can we have an Affine/Traits.h that defines this and just include it in Function.h? It isn't ideal, but we already have to do this for interfaces anyways? I'm just trying to avoid the future where many non-core traits get pulled into IR/ because of the current weirdness with FuncOp.
Sure - that sounds good. Done. 


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:104
   if (auto arg = value.dyn_cast<BlockArgument>())
-    return isFunctionRegion(arg.getOwner()->getParent());
-  return isFunctionRegion(value.getDefiningOp()->getParentRegion());
+    return arg.getOwner()->getParentOp() == region->getParentOp();
+  return value.getDefiningOp()->getParentOp() == region->getParentOp();
----------------
rriddle wrote:
> `value.getParentRegion() == region`?
Thanks. 


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:225
+
+// Value can be used as a symbol for `region` if it is a constant, or is defined
+// at the top level of 'region' or is its argument, or dominates `region`'s
----------------
andydavis1 wrote:
> There are a lot of "ors" in this description, and this is the place most people will be looking for what the definition of what a symbol is.  Could you make this a bullet list:
> // Value can be used as a symbol iff it meets one of the following conditions:
> // *) It is a constant.
> // *) It is defined at the top level of 'region'...
> // *) ...
Sure. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78863





More information about the llvm-commits mailing list