Page MenuHomePhabricator

[MLIR] Introduce op trait PolyhedralScope (revised)
ClosedPublic

Authored by bondhugula on Apr 28 2020, 6:51 PM.

Details

Summary

(A previous version of this, dd2c639c3cd397dfef941186fb85c82e4e918425, was
reverted.)

Introduce op trait PolyhedralScope for ops to define a new scope for
polyhedral optimization / affine dialect purposes, thus generalizing
such scopes beyond FuncOp. Ops to which this trait is attached will
define a new scope for the consideration of SSA values as valid symbols
for the purposes of polyhedral analysis and optimization. Update methods
that check for dim/symbol validity to work based on this trait.

Diff Detail

Event Timeline

bondhugula created this revision.Apr 28 2020, 6:51 PM
Herald added 1 blocking reviewer(s): rriddle. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

ModuleOp is now marked with this trait as well. The trait lives in IR - as discussed here (https://reviews.llvm.org/D78863?id=260322#inline-722790), the trait makes sense even for those that don't depend on the affine dialect, and it's clean/logical to mark FuncOp/ModuleOp with this trait. The cyclic dependence b/w Affine and IR/ is now eliminated.

bondhugula retitled this revision from [MLIR] Introduce op trait PolyhedralScope to [MLIR] Introduce op trait PolyhedralScope (revised).Apr 28 2020, 6:54 PM

add test case where there is no surrounding func op

ftynse accepted this revision.Apr 29 2020, 12:59 AM

ModuleOp is now marked with this trait as well. The trait lives in IR - as discussed here (https://reviews.llvm.org/D78863?id=260322#inline-722790), the trait makes sense even for those that don't depend on the affine dialect, and it's clean/logical to mark FuncOp/ModuleOp with this trait. The cyclic dependence b/w Affine and IR/ is now eliminated.

I don't think we reached final consensus on this point, but I vote for landing this regardless of the current implementation and discussing the layering separately on discourse. (If @rriddle is about to split OpTraits into several parts, all traits will then move in a consistent way).

Please wait for @rriddle to approve.

ModuleOp is now marked with this trait as well. The trait lives in IR - as discussed here (https://reviews.llvm.org/D78863?id=260322#inline-722790), the trait makes sense even for those that don't depend on the affine dialect, and it's clean/logical to mark FuncOp/ModuleOp with this trait. The cyclic dependence b/w Affine and IR/ is now eliminated.

I don't think we reached final consensus on this point, but I vote for landing this regardless of the current implementation and discussing the layering separately on discourse. (If @rriddle is about to split OpTraits into several parts, all traits will then move in a consistent way).

Please wait for @rriddle to approve.

I agree that there may be a better place for this trait to reside. But since a split may be in the works for other traits as well, we can have that happen separately first.

rriddle accepted this revision.Apr 29 2020, 1:41 AM

I'm fine with landing for now, but it should really be moved out when possible.

This revision is now accepted and ready to land.Apr 29 2020, 1:41 AM
This revision was automatically updated to reflect the committed changes.

Would it make sense to call this "AffineScope". I don't believe we use the word "Polyhedral" anywhere. This was intentional because of the many preexisting notions of what "Polyhedral" means, but in any case we should be consistent.

Would it make sense to call this "AffineScope". I don't believe we use the word "Polyhedral" anywhere. This was intentional because of the many preexisting notions of what "Polyhedral" means, but in any case we should be consistent.

Yes, AffineScope would appear to be more consistent. Sounds good to rename.