Page MenuHomePhabricator

[MLIR] Consider AffineIfOp when getting the index set of an Op wrapped in nested loops
ClosedPublic

Authored by kumasento on Mon, Jul 27, 12:59 PM.

Details

Summary

This diff attempts to resolve the TODO in getOpIndexSet (formerly known as getInstIndexSet), which states "Add support to handle IfInsts surronding op".

Major changes in this diff:

  1. Overload getIndexSet. The overloaded version considers both AffineForOp and AffineIfOp.
  2. The getInstIndexSet is updated accordingly: its name is changed to getOpIndexSet and its implementation is based on a new API getIVs instead of getLoopIVs.
  3. Add addAffineIfOpDomain to FlatAffineConstraints, which extracts new constraints from the integer set of AffineIfOp and merges it to the current constraint system.
  4. Update how a Value is determined as dim or symbol for ValuePositionMap in buildDimAndSymbolPositionMaps.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bondhugula added inline comments.Sat, Aug 1, 3:50 AM
mlir/include/mlir/Analysis/AffineAnalysis.h
41

Please continue on previous line. Put ops in back ticks.

mlir/include/mlir/Analysis/AffineStructures.h
222

This line may be confusing. An IntegerSet doesn't have any SSA values with itself. Just drop the "Since an IntegerSet ..., " part and just have the second clause to the effect: "The ifOp's operands are bound to the corresponding ... ".

224

If this only returns success now, please remove the LogicalResult. Also, mention about the assertion here about the unhandled cases. (For eg. IntegerSet having semi-affine expresssions.)

mlir/lib/Analysis/AffineAnalysis.cpp
93

Avoid auto.

103

Avoid auto here.

116

Triple /// please.

578

Comment here - not obvious.

584

Debug code.

605

Avoid auto.

mlir/lib/Analysis/Utils.cpp
50

You can use variadic isa: (isa<..., ...>).

mlir/lib/Transforms/Utils/LoopUtils.cpp
2339

Avoid auto.

This revision now requires changes to proceed.Sat, Aug 1, 3:50 AM
kumasento updated this revision to Diff 282389.Sat, Aug 1, 5:05 AM
kumasento marked 14 inline comments as done.

Fixed issues pinpointed by review comments.

kumasento marked an inline comment as done.Sat, Aug 1, 5:06 AM

Thank you very much Uday for pointing out all these issues! Sorry for costing you much time for doing so. I have fixed all of them and some other similar things. Would you mind taking another round of review to see if it is OK? Thanks!

mlir/include/mlir/Analysis/AffineStructures.h
222

So sorry for the confusion. As you said that an IntegerSet indeed doesn't have any SSA values, however, the columns in the constraint system should be bound to values. Here what I'm trying to say is that I bind ifOp's operands to those columns to workaround this problem.

But I suppose this point might be too trivial. I would just remove this sentence then. Please let me know if you prefer it stays there (revised based on your comments).

kumasento marked an inline comment as not done.Sat, Aug 1, 5:07 AM
bondhugula requested changes to this revision.Sat, Aug 1, 7:30 AM
bondhugula added inline comments.
mlir/include/mlir/Analysis/Utils.h
42

This doc comment isn't clear. There is no IV for an affine.if - please rewrite this doc.

mlir/lib/Analysis/AffineAnalysis.cpp
578

"a list of parent blocks" is confusing. Instead, the chain of ancestor blocks?

579

-> ... when either a FuncOp or endBlock is reached. ?

586

Instead of checking for FuncOp, check for an op with trait AffineScope (using hasTrait). FuncOp has AffineScope, but there are other ops also that define an AffineScope.

597

commonForValue -> commonForIV

mlir/lib/Analysis/AffineStructures.cpp
731

mlir::Value -> Value

mlir/lib/Analysis/Utils.cpp
49

You may want to do a ops->clear() in the beginning.

This revision now requires changes to proceed.Sat, Aug 1, 7:30 AM
kumasento updated this revision to Diff 282398.Sat, Aug 1, 8:08 AM
kumasento marked 7 inline comments as done.

Improved doc and API design based on reviews.

Thank you again Uday for your reviews! Please check again if I've missed any other things. :)

mlir/lib/Analysis/AffineAnalysis.cpp
586

Thanks! Can I say that the idea is not to go out of the current AffineScope?

bondhugula added inline comments.Sat, Aug 1, 8:50 AM
mlir/lib/Analysis/AffineAnalysis.cpp
586

That's exactly right.

kumasento updated this revision to Diff 282435.Sun, Aug 2, 2:32 AM

Improved comments in buildDimAndSymbolPositonMaps.

kumasento retitled this revision from [MLIR][Affine] Take into account `affine.if` in `getInstIndexSet`. to [MLIR][Affine] Take into account AffineIfOp when getting the index set of an operation wrapped in nested loops.Sun, Aug 2, 2:41 AM
kumasento edited the summary of this revision. (Show Details)
kumasento retitled this revision from [MLIR][Affine] Take into account AffineIfOp when getting the index set of an operation wrapped in nested loops to [MLIR] Consider AffineIfOp when getting the index set of an operation wrapped in nested loops.
kumasento retitled this revision from [MLIR] Consider AffineIfOp when getting the index set of an operation wrapped in nested loops to [MLIR] Consider AffineIfOp when getting the index set of an Op wrapped in nested loops.

Hi Uday @bondhugula sorry to bother but would you mind giving this diff one more round of review when you've got time? Thanks!

bondhugula requested changes to this revision.Thu, Aug 6, 7:18 AM
bondhugula added inline comments.
mlir/include/mlir/Analysis/AffineStructures.h
219–220

Reflow to use entire width.

mlir/lib/Analysis/AffineAnalysis.cpp
233

In another word -> In other words

know -> do not know ?

313

Document isSymbolDetermined please.

mlir/lib/Analysis/Utils.cpp
50

The name here would be incorrect. If you have AffineIfOp's interleaved, your output is no longer IVs (induction variables). This should be named more like getEnclosingAffineForAndIfOps. In fact, all users of this that just expect IVs would behave wrongly when given an input that has both for's and if's surrounding op.

This revision now requires changes to proceed.Thu, Aug 6, 7:18 AM
kumasento updated this revision to Diff 283617.Thu, Aug 6, 8:15 AM
kumasento marked 3 inline comments as done.
  1. Changed the naming of getIVs to getEnclosingAffineForAndIfOps
  2. Improved updateValuePosMap documentation
  3. Added comments to isSymbolDetermined

Thank you very much Uday @bondhugula for all your help! I've just improved the diff based on your reviews, would you mind taking a look again when you have time? Thanks!

mlir/include/mlir/Analysis/AffineStructures.h
219–220

Thanks! Would it be ok that I place the bullet points in separate lines?

mlir/lib/Analysis/AffineAnalysis.cpp
233

Thank you very much for this suggestion! I realized that this version is still not clear, and I try to rephrase it with more details. Hope this version has much less confusion.

mlir/lib/Analysis/Utils.cpp
50

Thank you for pointing this out! I was confused by the naming as well and I should have raised this issue earlier. I think getEnclosingAffienForAndIfOps is a clear name. I just changed getIVs to that :)

kumasento added inline comments.Thu, Aug 6, 1:46 PM
mlir/lib/Analysis/AffineAnalysis.cpp
244

I'm not very confident in this statement. It is based on an assumption that an operand from srcAccessMap or dstAccessMap should not only appear in affine.if. If not, I'm not sure how I could distinguish whether an operand is a dim or a symbol.

bondhugula accepted this revision.Fri, Aug 7, 3:14 AM

Looks good, thanks.

mlir/lib/Transforms/Utils/LoopUtils.cpp
2341

You don't need getOperation() here I think.

This revision is now accepted and ready to land.Fri, Aug 7, 3:14 AM
kumasento updated this revision to Diff 283868.Fri, Aug 7, 4:19 AM
kumasento marked an inline comment as done.

Removed redundant getOperation().

Thank you Uday! @bondhugula Would you mind helping me commit this if it looks OK :)

bondhugula accepted this revision.Fri, Aug 7, 9:06 PM
bondhugula requested changes to this revision.
bondhugula added inline comments.
mlir/include/mlir/Analysis/AffineAnalysis.h
37

This sentence is no longer accurate: "... of the forOps ...".

This revision now requires changes to proceed.Fri, Aug 7, 9:07 PM
bondhugula accepted this revision.Fri, Aug 7, 9:12 PM
bondhugula added inline comments.
mlir/include/mlir/Analysis/AffineAnalysis.h
38

Please also mention here that constraints from AffineIfOp's index set are also added.

mlir/include/mlir/Analysis/Utils.h
44

This comment is also no longer accurate:

-> "... outermost `affine.for/affine.if operation to ..."

mlir/lib/Analysis/AffineAnalysis.cpp
585

Please add a comment for this while loop here.

This revision is now accepted and ready to land.Fri, Aug 7, 9:12 PM
kumasento updated this revision to Diff 284122.Sat, Aug 8, 12:32 AM

Improved comments.

kumasento updated this revision to Diff 284123.Sat, Aug 8, 12:35 AM

Improved comments of getIndexSet.

Hi Uday @bondhugula Thank you for your latest review :) Please see whether the latest version looks better.

bondhugula accepted this revision.Sat, Aug 8, 9:44 AM

Some minor fixes.

mlir/include/mlir/Analysis/AffineAnalysis.h
40–41

values in AffineIfOps' index set -> AffineIfOp's operands

42

-> from the index sets of AffineIfOps ...

kumasento updated this revision to Diff 284135.Sat, Aug 8, 9:50 AM

Minor fixes in comments.

Some minor fixes.

Done :) Thank you Uday @bondhugula

bondhugula accepted this revision.Sat, Aug 8, 2:40 PM