This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kumasento on Jul 27 2020, 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

kumasento created this revision.Jul 27 2020, 12:59 PM
rriddle added inline comments.Jul 27 2020, 1:06 PM
mlir/include/mlir/Analysis/AffineAnalysis.h
45

nit: Please use /// for top-level comments. Same applies to the rest of this commit.

kumasento updated this revision to Diff 281053.Jul 27 2020, 2:09 PM

Fixed top-level comment problems and reverted AffineOps.cpp changes.

kumasento marked an inline comment as done.Jul 27 2020, 2:55 PM

Thanks very much for completing these! I'll be able to review this tomorrow. You could consider adding a few test cases to exercise these. These changes could be tested by adding test cases with affine.if's for say memref dep analysis or memref-bound-checking or affine data copy generation - whichever appears easier.

mlir/lib/Analysis/AffineAnalysis.cpp
134

This can be named getOpIndexSet now - the inst is a remnant of the past when Operations in MLIR used to be called Instructions and other things.

kumasento updated this revision to Diff 281486.Jul 29 2020, 2:58 AM
  1. Added unittest for memref-dependence-check with affine.if
  2. Fixed issues in getCommonBlock that doesn't consider the existence of affine.if
  3. Fixed issues in addAffineIfDomain that doesn't consider local variable
  4. Changed getInstIndexSet naming to getOpIndexSet

Thanks very much for completing these! I'll be able to review this tomorrow. You could consider adding a few test cases to exercise these. These changes could be tested by adding test cases with affine.if's for say memref dep analysis or memref-bound-checking or affine data copy generation - whichever appears easier.

Thank you very much Uday for your comments :)

Now I've added some unittests based on test-memref-dependence-check. They are in a separate file but I can put them into memref-dependence-check.mlir if you prefer.

kumasento updated this revision to Diff 281488.Jul 29 2020, 3:03 AM

Squashed recent commits into one.

kumasento abandoned this revision.Jul 29 2020, 6:04 AM
kumasento reclaimed this revision.
bondhugula requested changes to this revision.Jul 29 2020, 10:13 PM

Looks great so far - thanks. Some more comments.

mlir/include/mlir/Analysis/AffineAnalysis.h
36–50

We really don't need two versions - the one you added should be the single version and the right one to use. Please merge these two into one?

mlir/lib/Analysis/AffineAnalysis.cpp
102–105

Shouldn't you be asserting that everything in ops is either an AffineForOp or an AffineIfOp?

128

opInst is outdated - update doc comment.

263–268

Can drop trivial braces here.

mlir/test/Transforms/memref-dependence-check-with-if.mlir
1 ↗(On Diff #281488)

Can you append these to the existing file on dependence tests?

This revision now requires changes to proceed.Jul 29 2020, 10:13 PM
kumasento updated this revision to Diff 281826.Jul 30 2020, 1:36 AM

Improved API design, comments, and unittests.

kumasento marked 5 inline comments as done.Jul 30 2020, 1:38 AM

Hi @bondhugula Thank you for your comments. I've updated this diff according to your suggestions.

kumasento added inline comments.Jul 30 2020, 1:40 AM
mlir/lib/Dialect/Affine/Transforms/LoopTiling.cpp
221 ↗(On Diff #281826)

BTW I'm not sure whether there is any way to make this ops creation a bit more concise?

bondhugula requested changes to this revision.Aug 1 2020, 3:50 AM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Transforms/LoopTiling.cpp
219 ↗(On Diff #281826)

Please reserve space for ops. ops.reserve(input.size()).

220 ↗(On Diff #281826)

Please avoid auto here.

221 ↗(On Diff #281826)

This is fine.

mlir/lib/Transforms/Utils/LoopUtils.cpp
2338 ↗(On Diff #281826)

Likewise. Reserve space please.

2339 ↗(On Diff #281826)

Avoid auto.

2425 ↗(On Diff #281826)

2 -> 1 ?

bondhugula added inline comments.Aug 1 2020, 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.

94

Avoid auto here.

129

Triple /// please.

594

Comment here - not obvious.

600

Debug code.

610

Avoid auto.

mlir/lib/Analysis/Utils.cpp
50

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

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

Fixed issues pinpointed by review comments.

kumasento marked an inline comment as done.Aug 1 2020, 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.Aug 1 2020, 5:07 AM
bondhugula requested changes to this revision.Aug 1 2020, 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
594

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

595

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

602

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.

602

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.Aug 1 2020, 7:30 AM
kumasento updated this revision to Diff 282398.Aug 1 2020, 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
602

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

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

That's exactly right.

kumasento updated this revision to Diff 282435.Aug 2 2020, 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.Aug 2 2020, 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.Aug 6 2020, 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
248

In another word -> In other words

know -> do not know ?

329

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.Aug 6 2020, 7:18 AM
kumasento updated this revision to Diff 283617.Aug 6 2020, 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
248

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.Aug 6 2020, 1:46 PM
mlir/lib/Analysis/AffineAnalysis.cpp
259

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.Aug 7 2020, 3:14 AM

Looks good, thanks.

mlir/lib/Transforms/Utils/LoopUtils.cpp
2341 ↗(On Diff #283617)

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

This revision is now accepted and ready to land.Aug 7 2020, 3:14 AM
kumasento updated this revision to Diff 283868.Aug 7 2020, 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.Aug 7 2020, 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.Aug 7 2020, 9:07 PM
bondhugula accepted this revision.Aug 7 2020, 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
601

Please add a comment for this while loop here.

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

Improved comments.

kumasento updated this revision to Diff 284123.Aug 8 2020, 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.Aug 8 2020, 9:44 AM

Some minor fixes.

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

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

42

-> from the index sets of AffineIfOps ...

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

Minor fixes in comments.

Some minor fixes.

Done :) Thank you Uday @bondhugula

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