Support affine.parallel in the index set analysis. It allows us to do dependence analysis containing affine.parallel in addition to affine.for and affine.if. This change only supports the constant lower/upper bound in affine.parallel. Other complicated affine map bounds will be supported in further commits.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Some general code-style comments to start the review.
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
---|---|---|
246–248 | Please update this message. | |
254–255 | Any reason for not using else if? | |
271–273 | nit: remove braces here. | |
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp | ||
654 | Is there a reason for using auto here and not the actual type? I think generally the actual type is prefered. | |
655–660 | nit: remove braces here. | |
mlir/lib/Dialect/Affine/IR/AffineOps.cpp | ||
2330–2331 | nit: there is no need for a newline before the if. | |
2332–2334 | nit: remove braces. |
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
---|---|---|
647–649 | I don't think this ordering is valid for affine.parallel since it does not define any lexicographic order. For example: affine.parallel (%i) = (0) to (10) { %1 = affine.load %m[9 - %i] affine.store %1, [%i] } There can be a dependence from %i = 9 to %i = 0, since no lexicographic order is defined. |
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
---|---|---|
647–649 | I see. We may need to skip this constraint in the case of affine.parallel in the middle. |
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
---|---|---|
647–649 | I think there should be a dependency from the store to write (e.g. store %m[0] (%i=0) to load %m[0] (%i = 9). But this case is not properly checked. func.func @no_lexicographic_order() { %m = memref.alloc() : memref<10xf32> %c7 = arith.constant 7.0 : f32 affine.parallel (%i) = (0) to (10) { %1 = affine.load %m[9 - %i] : memref<10xf32> // expected-remark@above {{dependence from 0 to 0 at depth 1 = false}} // expected-remark@above {{dependence from 0 to 1 at depth 1 = true}} affine.store %1, %m[%i] : memref<10xf32> // expected-remark@above {{dependence from 1 to 0 at depth 1 = false}} // expected-remark@above {{dependence from 1 to 1 at depth 1 = false}} } return } |
Thanks for contributing this support. This was long overdue. While this is fine, I feel just directly supporting the general (arbitrary bound case) is better -- since the affine.for support already supports that, and we may have to again throw away most of this code when handling the general bound case. Just supporting the constant bound case to start with isn't an incremental step.
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
---|---|---|
253 | Use comments on top (LLVM/MLIR style), not trailing. | |
261 | May be good to fix the comment while on this: | |
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp | ||
650 | Improve the assertion message here: variable expected for the IV value? | |
655–658 | Negate condition and unguard addBound. | |
661–664 | Likewise. |
@bondhugula Thank you for the comments. Sorry for taking the time to work on this. I was struggling with how to treat the ordering constraint in the affine.parallel. (Just removing the ordering constraint does not work as expected).
So for now, should we discard this patch completely and restart working on supporting general case?
It would be good to support the generic case. Parts of this patch are still useful towards that. I didn't understand the issue related to "ordering constraint". You would order the dimensions in the order in which they appear in the affine.parallel.
I think the "ordering constraint" refers to the dependence analysis, since affine.parallel does not define a schedule for iterations, we cannot impose ordering constraints based on lexicographic order as it currently does for every induction variable.
I think the patch is good other than the dependence analysis part since that needs more discussion. For example, checkMemRefDependence takes a loop depth. Currently, loop depth implies the induction variable at that position. What would that mean after introducing affine.paralllel to the dependence analysis? Would it mean after affine.parallel is allowed? Would it mean the loop or the induction variable? I think some things need to be discussed more before we support it.
I would be happy to review the patch if you send the generic case without dependence analysis.
@bondhugula @Groverkss Thanks again. I'd like to work on the general cases for supporting affine.parallel in the getIndexSet.
But I have one question about the test for that case. Where should we write the test if we do not support the dependence analysis for affine.parallel for the time being? Is there any existing test checking the getIndexSet functionality?
If we don't support it temporarily, you can always return failure() from getIndexSet which will cleanly fail the dependence analysis check. There is a status for "dependence analysis failed" that gets returned already.
@bondhugula Okay, I'll return failure() to unsupport the dependence analysis with affine.parallel for the time being. Thanks!
mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h | ||
---|---|---|
148 | Is this stil lvalid? | |
149 | Is it required that the variable exists in the constraint system? The documentation does not make this clear. | |
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
617 | This statement is misleading. Dependence analysis isn't implemented for affine.parallel. This statement claims it's not possible. Please add a TODO instead explaining why we did not implement it right now, which is affine.parallel does not specify an ordering. | |
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp | ||
654 | Is there any reason we specialize for the constant case? If there is, could you write a comment explaining why we specialize? It is not obvious to me. |
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp | ||
---|---|---|
654 | The addBound method depends on whether the bound is constant. In the case of non-constant, we need to give operands explicitly. We use this pattern for affine.for and it's also the case for affine.parallel too. |
The added code isn't exercised by any test case, right? Is this ready for review?
mlir/include/mlir/Dialect/Affine/IR/AffineOps.h | ||
---|---|---|
455 | What can operations contain? operations -> affineOps? | |
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp | ||
273 | Can use auto here since the type is on the RHS. | |
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp | ||
646 | The debug message isn't helpful here. Suggest adding more text to it. | |
mlir/lib/Dialect/Affine/IR/AffineOps.cpp | ||
259–264 | This is nothing but checking getParentOpOfType<AffineParallelOp>() is null. We don't need to add a public method! | |
2325 | Pass output argument by reference (MLIR convention). | |
2329 | Use auto here. | |
2331 | Use braces here per new LLVM/MLIR style. |
Thank you for taking a look again.
I'll try to add a test case covering getIndexSet somehow. But there seems to be no dedicated test case for getIndexSet. Test cases for dependent analysis only check the logic of getIndexSet. Do you know whether there is already any test code covering getIndexSet usage?
I didn't understand - you don't need to add test cases for the utility. Just test dependence analysis in the presence of affine.parallel. This can be done via the affine-scalrep pass or test-memref-dependence-check.
Sorry for the confusion. I just wanted to know how/where to add test cases for the utility functions like this. But I've found a way to test the case including affine.parallel somehow for dependence check and scalrep. I'm so sorry for bothering you many times but could you review this change again when you get a chance?
Can you please resolve the other remaining comments and mark them done before a round of review? For eg. all comments in AffineStructures.cpp many of which are trivial are still pending.
mlir/lib/Dialect/Affine/IR/AffineOps.cpp | ||
---|---|---|
2332–2334 | Drop trivial braces. | |
mlir/test/Transforms/memref-dependence-check.mlir | ||
1078–1080 | There seems to be something incorrect here. Both these "expected" checks are pointing to the same line (@above from below is the same as +1 here). | |
mlir/test/lib/Analysis/TestMemRefDependenceCheck.cpp | ||
90 ↗ | (On Diff #477855) | Thank you for improving/fixing this. |
mlir/test/Transforms/memref-dependence-check.mlir | ||
---|---|---|
1078–1080 | We should have checked the affine.parallel existence before returning no dependence when there is no AffineWriteOpInterface. |
This looks great to me - thanks! A comment on some missing test cases below.
mlir/test/Dialect/Affine/scalrep.mlir | ||
---|---|---|
796 ↗ | (On Diff #478805) | Missing test case for non-constant loop bound case and mixed affine.parallel + affine.for case. (For eg., affine.for surrounded by affine.parallel or vice versa) -- either here or in the dependence check test case file. |
Is this stil lvalid?