Page MenuHomePhabricator

[mlir][Linalg] emitLoopRanges and emitLoopRangesWithSymbols merged into one
ClosedPublic

Authored by limo1996 on Tue, Jul 14, 2:53 AM.

Details

Summary

Right now there is a branching for 2 functions based on whether target map has
symbols or not. In this commit these functions are merged into one.
Furthermore, emitting does not require inverse and map applying as it computes
the correct Range in a single step and thus reduces unnecessary overhead.

Diff Detail

Event Timeline

limo1996 created this revision.Tue, Jul 14, 2:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 14, 2:53 AM
bondhugula added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
115

Avoid auto here.

527

Avoid repeated evaluation of map.getNumSymbols().

limo1996 updated this revision to Diff 278204.Wed, Jul 15, 8:34 AM

comments of bondhugula incorporated

limo1996 marked 2 inline comments as done.Wed, Jul 15, 8:37 AM

Comments of bondhugula marked as done

bondhugula added inline comments.Wed, Jul 15, 8:32 PM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
61–71

Nit: if it contains some -> if it contains any

limo1996 updated this revision to Diff 278382.Wed, Jul 15, 11:48 PM

some -> any

limo1996 marked an inline comment as done.Wed, Jul 15, 11:48 PM

Can you please elaborate in the commit description and/or code comments _how_ this avoids inversion? The code seems to just pass in the direct map to the range calculation function, which takes result sizes in order. I don't see why this would work, but I may be missing something.

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
61–62

Could you please elaborate _which_ expressions are supported?

83

The change from d.getPosition to idx looks wrong. The former returns the index of the affine dimension in the expression and the latter is merely an index of the expression in the map. Has this not broken any tests?

100–110

Any chance of finding better names than m and n?

113

The RHS of floordiv is not necessarily 2 in the code, but it must be some constant AFAICS, please check for that.

525–526

Please terminate sentences with a full stop in comments.

ftynse requested changes to this revision.Thu, Jul 16, 3:59 AM
This revision now requires changes to proceed.Thu, Jul 16, 3:59 AM
ftynse added inline comments.Thu, Jul 16, 7:00 AM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
83

After a further investigation, the code on which this is based is wrong, but the updated code looks correct.

91

Please address the comments on the diff that introduces this code, then rebase this diff.

limo1996 updated this revision to Diff 278471.Thu, Jul 16, 7:27 AM

ftynse's comments addressed

limo1996 marked 6 inline comments as done.Thu, Jul 16, 7:30 AM
limo1996 marked 2 inline comments as done.Fri, Jul 17, 9:03 AM
limo1996 added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
91

Done

limo1996 marked an inline comment as done.Fri, Jul 17, 9:03 AM
ftynse accepted this revision.Mon, Jul 20, 1:29 AM

Thanks for the cleanup!

This revision is now accepted and ready to land.Mon, Jul 20, 1:29 AM
limo1996 updated this revision to Diff 279278.Mon, Jul 20, 9:26 AM

Changes evolved and merged from parent commit

nicolasvasilache accepted this revision.Wed, Jul 22, 1:49 AM

Looks great, thanks!

This revision was automatically updated to reflect the committed changes.