Page MenuHomePhabricator

[mlir] Prevent operations with users from being hoisted
ClosedPublic

Authored by sumesh13 on Thu, Apr 1, 12:36 PM.

Details

Summary

This patch collects operations that have users in a for loop and uses them when loop invariant operations are detected and hoisted.

Diff Detail

Event Timeline

sumesh13 created this revision.Thu, Apr 1, 12:36 PM
sumesh13 requested review of this revision.Thu, Apr 1, 12:36 PM
bondhugula requested changes to this revision.Thu, Apr 1, 8:40 PM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
187–188

This choice of name definedOps appears to be confusing: there is no such thing as ops being defined. It would be good rename this and add a comment while on this (since you are adding to definedOps below) to make this readable.

197

Nit: op.getNumResults() > 0 if you prefer.

This revision now requires changes to proceed.Thu, Apr 1, 8:40 PM
sumesh13 updated this revision to Diff 334962.Fri, Apr 2, 8:37 AM
  1. Address code review comments including renaming definedOps to opsWithusers
sumesh13 marked 2 inline comments as done.Mon, Apr 5, 1:42 PM
sumesh13 added inline comments.
mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
187–188

I have renamed it to opsWithUsers

sumesh13 marked an inline comment as done.Wed, Apr 7, 8:27 PM

Gentle ping

bondhugula requested changes to this revision.Wed, Apr 7, 8:42 PM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
197

Just because the op has results doesn't mean those results have users. You can do a check in O(1) time to determine if the results have no uses (use_empty()). This will make the check more powerful and you'll still be able to hoist those that have results but are ultimately unused.

This revision now requires changes to proceed.Wed, Apr 7, 8:42 PM
vinayaka-polymage requested changes to this revision.Wed, Apr 7, 11:01 PM

Thank you for bringing up this issue in LICM, it is an important one. I have a few comments on the fix:

  1. Inherent issue is definedOps ( going to be renamed to opsWithUsers) is being populated only under some conditions, and does not actually do the job of collecting all ops that define an SSA value in that block.
  2. Even after this fix, only AffineForOps will be added to the list, but there are other cases like AffineDma... ops, AffineIfOps that are actually NOT code invariant etc. which will still cause similar problems.

So, I suggest that you can implement a generic fix for this inherent issue, where any op that defines an SSA value can be added to the above list, and this is decoupled from the checks like 'isOpLoopInvariant`. This will make the check more robust, and relevant test cases can also be added.

Minor suggestion.

mlir/test/Dialect/Affine/affine-loop-invariant-code-motion.mlir
620

Minor suggestion: It is better to create a minimal test case that captures the intended check. A similar test case without vector_load and vector.reduction can be created to improve readability.

sumesh13 retitled this revision from [mlir] Prevent AffineFor users from being hoisted to [mlir] Prevent operations with users from being hoisted.Sun, Apr 11, 11:12 AM
sumesh13 edited the summary of this revision. (Show Details)
sumesh13 updated this revision to Diff 336681.Sun, Apr 11, 11:15 AM

Rework based on comments

  • Separate the part that identifies operations that have users and use it in the later phase.

Thank you for bringing up this issue in LICM, it is an important one. I have a few comments on the fix:

  1. Inherent issue is definedOps ( going to be renamed to opsWithUsers) is being populated only under some conditions, and does not actually do the job of collecting all ops that define an SSA value in that block.
  2. Even after this fix, only AffineForOps will be added to the list, but there are other cases like AffineDma... ops, AffineIfOps that are actually NOT code invariant etc. which will still cause similar problems.

So, I suggest that you can implement a generic fix for this inherent issue, where any op that defines an SSA value can be added to the above list, and this is decoupled from the checks like 'isOpLoopInvariant`. This will make the check more robust, and relevant test cases can also be added.

Thanks @vinayaka-polymage for the very useful feedback. Hadnt realized the issue might affect other operations such as Affineif etc. I have reworked the code and also addes two tests.

sumesh13 marked 2 inline comments as done.Sun, Apr 11, 11:19 AM

I have addresses the comments. Pls take a look.

vinayaka-polymage requested changes to this revision.Sun, Apr 11, 10:04 PM

@sumesh13 This new generic fix looks good, it handles more cases that were previously not handled. Thanks for this. I have added some comments related to code organization, please check them. Logic looks good to me.

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
48

It is better to keep this local like before, see comment below.

190

This is a per AffineForOp var, and can be kept local here like before.

201

Populating into definedOps (opsWithUsers) can happen here. No need to walk the affine for, because source of an operand for any op, can only come before it. So,

if (op->getNumResults() > 0 && !op->use_empty())
        opsWithUsers.insert(op);

can be placed here.

This revision now requires changes to proceed.Sun, Apr 11, 10:04 PM
sumesh13 updated this revision to Diff 336858.Mon, Apr 12, 9:00 AM

Address review comments on code reorg

@vinayaka-polymage Thanks for the comments. Pls take a look

sumesh13 marked 2 inline comments as done.Mon, Apr 12, 9:03 AM
sumesh13 updated this revision to Diff 336860.Mon, Apr 12, 9:15 AM

Undo a change w.r.t to the base that had crept in

Mark review comments as done and undo a typo that had crept in

@sumesh13 Thanks for addressing all the comments, the change looks good now. Can you please check these last two comments before I approve this ?

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
81–82

Don't need this now, right ? This line can be deleted, and comment be moved to the location where it is getting populated.

226–227

This section now does not have any change, so please make sure that there is no diff here.

Please change the commit title to a more relevant one, I am approving this.

@bondhugula Could you please take a final pass at this?

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
81–82

Actually, I take my above comment back. This function is being used in ifOp hoisting as well, and with the current implementation it will be needed. Refactoring that can be a separate change in itself.

sumesh13 updated this revision to Diff 336879.Mon, Apr 12, 10:03 AM
sumesh13 edited the summary of this revision. (Show Details)

Address comments

@vinayaka-polymage Just noticed that you accepted a previous revision with comment on handling of ifops. Considering ifops, would you think it might be helpful to just collect all the ops like I was doing in a previous revision ?

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
81–82

Thanks ...that was one reason I was collecting ops separately. Would you think that might be preferable ?

sumesh13 updated this revision to Diff 336881.Mon, Apr 12, 10:13 AM

Revert to earlier revision that was approved and some minor cleanup of comments.

sumesh13 marked 3 inline comments as done.Mon, Apr 12, 10:15 AM

Adding back a change from previous revision that was approved and some minor cleanup.

Gentle reminder..also pls suggest if it would be better to collect all the opsWithUsers separately as i was doing in a previous revision. For nested ifs, looks like otherwise you may not be collecting all the opsWithUsers by just collecting at the top level and leaves.

bondhugula added inline comments.Tue, Apr 13, 10:27 AM
mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
199

I think just !op.use_empty() should be sufficient.

sumesh13 updated this revision to Diff 337203.Tue, Apr 13, 10:45 AM

Simplify condition to collect ops with users.

sumesh13 marked an inline comment as done.Tue, Apr 13, 10:46 AM

Address comment on simplifying condition to add ops with users.

bondhugula accepted this revision.Tue, Apr 13, 11:12 AM

Looks great.

This revision is now accepted and ready to land.Tue, Apr 13, 11:12 AM

@bondhugula , @vinayaka-polymage thanks for the review. @sgrechanik Can you pls help land this

This revision was landed with ongoing or failed builds.Tue, Apr 13, 3:38 PM
This revision was automatically updated to reflect the committed changes.