This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Wrong tile size for convolutions fixed
ClosedPublic

Authored by limo1996 on Aug 26 2020, 9:35 AM.

Details

Summary

Sizes of tiles (subviews) are bigger by 1 than they should. Let's consider
1D convolution without batches or channels. Furthermore let m iterate over
the output and n over the kernel then input is accessed with m + n. In tiling
subview sizes for convolutions are computed by applying requested tile size
together with kernel size to the above mentioned expression thus let's say
for tile size of 2 the subview size is 2 + size(n), which is bigger by one
than it should since we move kernel only once. The problem behind it is that
range is not turned into closed interval before the composition. This commit
fixes the problem by turning ranges first into closed intervals by substracting
1 and after the composition back to half open by adding 1.

PHAB_REVIEW=D86638

Diff Detail

Event Timeline

limo1996 created this revision.Aug 26 2020, 9:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 9:35 AM
limo1996 requested review of this revision.Aug 26 2020, 9:35 AM
mravishankar requested changes to this revision.Aug 26 2020, 10:14 AM
mravishankar added a reviewer: mravishankar.
mravishankar added a subscriber: mravishankar.

@limo1996 This is really nice find! I have been struggling with figuring out why convolutions are giving me errors in some cases, and I suspected something like, but wasnt able to nail this down.

There might be a better fix though. I'll take a look at this as well. For now I am going to just "Request changes" but I dont really have any changes to request. Just a placeholder for me to take a look again.

This revision now requires changes to proceed.Aug 26 2020, 10:14 AM

@limo1996 This is really nice find! I have been struggling with figuring out why convolutions are giving me errors in some cases, and I suspected something like, but wasnt able to nail this down.

There might be a better fix though. I'll take a look at this as well. For now I am going to just "Request changes" but I dont really have any changes to request. Just a placeholder for me to take a look again.

Thank you @mravishankar! I know for sure there is a better fix but my goal here was to initiate a discussion.

You can try new Convolutions for different layouts and ranks that were recently added. I also saw that traditional linalg.conv is quite buggy but the plan is that it will be replaced so I did not invest time into fixing it.

Could we start a discourse post on this. Its more easy to document issues and discuss solutions there rather than on the patch.

Also, I am curious what other bugs you found in the convolution operation. Please add those to the discourse post if possible.

Could we start a discourse post on this. Its more easy to document issues and discuss solutions there rather than on the patch.

Also, I am curious what other bugs you found in the convolution operation. Please add those to the discourse post if possible.

Here is the discourse post: https://llvm.discourse.group/t/wrong-tile-size-for-convolution-ops/1699

limo1996 edited the summary of this revision. (Show Details)Sep 2 2020, 5:20 AM
limo1996 retitled this revision from [mlir][WIP] Tiling for convolutions to [mlir][Linalg] Wrong tile size for convolutions fixed.
limo1996 edited the summary of this revision. (Show Details)
nicolasvasilache requested changes to this revision.Sep 2 2020, 5:48 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
248

Please use mlir::edsc::op::operator+ and write it as size + std_constant_index(1) to get the proper affine_apply operation generated.
Without that things won't canonicalize nicely.
See e.g. mlir/test/EDSC/builder-api-test.cpp line 82 for some C++ and the generated IR.

289

same thing here re operator+

mlir/test/Dialect/Linalg/tile_simple_conv.mlir
34

these shouls not appear as subi / addi but instead use affine_apply with the properly canonicalized/simplified expressions.

This revision now requires changes to proceed.Sep 2 2020, 5:48 AM
limo1996 updated this revision to Diff 289440.Sep 2 2020, 7:02 AM

Comments of nicolas resolved

limo1996 marked 3 inline comments as done.Sep 2 2020, 7:03 AM
nicolasvasilache accepted this revision.Sep 2 2020, 7:50 AM

Great, thank you @limo1996 !

mravishankar accepted this revision.Sep 2 2020, 9:16 AM

Thanks!

This revision is now accepted and ready to land.Sep 2 2020, 9:16 AM
This revision was automatically updated to reflect the committed changes.