This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Fix padding size calculation for Conv2d ops.
ClosedPublic

Authored by pzread on May 2 2022, 9:08 PM.

Details

Summary

This patch fixed the padding size calculation for Conv2d ops when the stride > 1. It contains the changes below:

  • Use addBound to add constraint for AffineApplyOp in getUpperBoundForIndex. So the result value can be mapped and retrieved later.
  • Fixed the bound from AffineMinOp by adding as a closed bound. Originally the bound was added as an open upper bound, which results in the incorrect bounds when we multiply the values. For example:
%0 = affine.min affine_map<()[s0] -> (4, -s0 + 11)>()[iv0]
%1 = affine.apply affine_map<()[s0] -> (s0 * 2)>()[%0]

If we add the affine.min as an open bound, addBound will internally transform it into the close bound "%0 <= 3". The following sliceBounds will derive the bound of %1 as "%1 <= 6" and return the open bound "%1 < 7", while the correct bound should be "%1 <= 8".
  • In addition to addBound, I also changed sliceBounds to support returning closed upper bound, since for the size computation, we usually care about the closed bounds.
  • Change the getUpperBoundForIndex to favor constant bounds when required. The sliceBounds will return a tighter but non-constant bounds, which can't be used for padding. The constantRequired option requires getUpperBoundForIndex to get the constant bounds when possible.

Diff Detail

Event Timeline

pzread created this revision.May 2 2022, 9:08 PM
Herald added a project: Restricted Project. · View Herald Transcript
pzread retitled this revision from [mlir][linalg] Fix padding size calculation for Conv2D ops. to [mlir][linalg] Fix padding size calculation for Conv2d ops..May 3 2022, 6:30 AM
pzread edited the summary of this revision. (Show Details)
pzread removed a reviewer: bondhugula.
pzread edited the summary of this revision. (Show Details)May 3 2022, 6:31 AM
pzread edited the summary of this revision. (Show Details)
pzread edited the summary of this revision. (Show Details)May 3 2022, 6:35 AM
pzread edited the summary of this revision. (Show Details)
pzread updated this revision to Diff 426688.May 3 2022, 6:54 AM
pzread edited the summary of this revision. (Show Details)

Improve comments.

pzread published this revision for review.May 3 2022, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 8:14 AM
hanchung accepted this revision.May 4 2022, 1:08 AM

Nice fix!

mlir/lib/Dialect/Linalg/Utils/Utils.cpp
259

[optional] we don't need the comment because the code (AffineMap::getConstantMap) describes it.

This revision is now accepted and ready to land.May 4 2022, 1:08 AM
pzread updated this revision to Diff 426970.May 4 2022, 5:30 AM

Remove unnecessary comments.

Groverkss added inline comments.May 5 2022, 6:27 AM
mlir/include/mlir/Dialect/Affine/Analysis/AffineStructures.h
177–178

Can you update the docs about what isClosedBound does? The requirement that isClosedBound needs to be true for BoundType::EQ is not obvious.

mlir/lib/Dialect/Linalg/Utils/Utils.cpp
256
pzread updated this revision to Diff 427319.May 5 2022, 7:40 AM

Improve docs.

pzread updated this revision to Diff 427326.May 5 2022, 7:50 AM

Replace the enum.

pzread marked 3 inline comments as done.May 5 2022, 7:50 AM
This revision was automatically updated to reflect the committed changes.