Page MenuHomePhabricator

[MLIR] Fixed missing append when adding an domain
ClosedPublic

Authored by kumasento on Aug 23 2020, 11:32 AM.

Details

Summary

The prior diff that introduced addAffineIfOpDomain forgot to append constraints from the ifOp domain. This diff fixed that problem.

Diff Detail

Event Timeline

kumasento created this revision.Aug 23 2020, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2020, 11:32 AM
kumasento requested review of this revision.Aug 23 2020, 11:32 AM
kumasento added inline comments.Aug 23 2020, 11:36 AM
mlir/test/Transforms/memref-dependence-check.mlir
20

Hi Uday @bondhugula

When taking into account the constraints introduced by affine.if, this affine.store statement will not be executed and there won't have dependences from 0 to 1. Please correct me if there is anything wrong.

If that is true, I'm wondering how would you prefer to make changes. Should I revise the test case as above, or should I change the dependence analysis behaviour, e.g., don't involve the domain from affine.if when checking dependences?

Thanks!

Hi Uday @bondhugula

Sorry that my prior diff D84698 has a stupid bug: I thought that mergeAndAlighIdsWithOthers would append constraints as well (merge seemed like an action on constraints not IDs), and apparently I was wrong. I need to use append to insert new constraints from the IfOp's domain. The test case that I introduced last time cannot find this kind of bug.

There are two things that I may need your input:

  1. It is possible that append may add some redundant constraints, is that OK?
  2. The old store_may_execute_before_load test case now have a different behaviour because we take into account the IfOp domain. How do you suggest we should update that test case?

Thanks!

Ruizhe

kumasento retitled this revision from [MLIR] Fixed missing append when adding IfOp domain to [MLIR] Fixed missing append when adding an domain.Aug 23 2020, 11:44 AM
bondhugula added inline comments.Aug 23 2020, 11:59 PM
mlir/test/Transforms/memref-dependence-check.mlir
20

You should definitely consider the domain of affine.if - that makes the dependence analysis precise. So you should revise the test case. :) Adding the constraints for any surrounding if's is the right thing.

Hi Uday @bondhugula

Sorry that my prior diff D84698 has a stupid bug: I thought that mergeAndAlighIdsWithOthers would append constraints as well (merge seemed like an action on constraints not IDs), and apparently I was wrong. I need to use append to insert new constraints from the IfOp's domain. The test case that I introduced last time cannot find this kind of bug.

There are two things that I may need your input:

  1. It is possible that append may add some redundant constraints, is that OK?

Yes, of course. You aren't in general expected to check in a scenario like this if the constraints you are adding are redundant - we don't in other cases. There are other simple O(N) methods to remove duplicate constraints like this (where N = number of constraints), and more complex methods to remove more complex redundancy.

  1. The old store_may_execute_before_load test case now have a different behaviour because we take into account the IfOp domain. How do you suggest we should update that test case?

The test case should definitely be updated given that the dependence analysis and any other routines using the same infra would now be more precise. I've responded to your other inline comment as well.

  • Uday

Thanks!

Ruizhe

bondhugula added inline comments.Aug 24 2020, 12:03 AM
mlir/lib/Analysis/AffineStructures.cpp
738

I missed this earlier in the review. We should have tested some of the infra that was affected (became more powerful) as a result of this.

mlir/test/Transforms/memref-dependence-check.mlir
1047

We'll need a comment here to explain the check (the increased upper bound) - otherwise, it would appear subtle.

bondhugula added inline comments.Aug 24 2020, 12:07 AM
mlir/test/Transforms/memref-dependence-check.mlir
1047

Please also add an extra test case either in this file or in memref-bound-check where you have a surrounding affine.if and the test is in bounds as a result of having considered the affine.if's domain. That would be a good positive test case for addAffineIfDomain. Note that those methods aren't able to deal with else yet (would still be imprecise), but under affine.if should be accounted for now.

kumasento updated this revision to Diff 288360.Thu, Aug 27, 8:58 AM
kumasento marked an inline comment as done.

Updated store_may_execute_before_load comments

mlir/test/Transforms/memref-dependence-check.mlir
20

Sure! I've revised the comments to reflect the new updates.

1047

Thank you Uday. It is a trivial update to fix a typo. Keeping 100 may cause confusion since there are 101 number of elements.

1047

Please also add an extra test case either in this file or in memref-bound-check where you have a surrounding affine.if and the test is in bounds as a result of having considered the affine.if's domain. That would be a good positive test case for addAffineIfDomain. Note that those methods aren't able to deal with else yet (would still be imprecise), but under affine.if should be accounted for now.

Thank you for pointing that test out! Currently I don't think memref-hound-check has considered affine.if. For example, when computing the MemRefRegion only constraints from loop IVs are involved.

I will fix this in another diff later.

bondhugula accepted this revision.Thu, Aug 27, 11:52 AM
bondhugula added inline comments.
mlir/test/Transforms/memref-dependence-check.mlir
13

-> indicates

This revision is now accepted and ready to land.Thu, Aug 27, 11:52 AM
bondhugula added inline comments.Thu, Aug 27, 11:56 AM
mlir/test/Transforms/memref-dependence-check.mlir
1047

Sounds good, thanks. You are right - MemRefRegion::compute actually needs an update.

I've committed this after taking care of the typo, a whitespace error, and typos in the commit title/summary.

I've committed this after taking care of the typo, a whitespace error, and typos in the commit title/summary.

Thank you Uday for taking care of them! I will improve memref-bound-check later.