This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by limo1996 on Jul 14 2020, 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.Jul 14 2020, 2:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 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.Jul 15 2020, 8:34 AM

comments of bondhugula incorporated

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

Comments of bondhugula marked as done

bondhugula added inline comments.Jul 15 2020, 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.Jul 15 2020, 11:48 PM

some -> any

limo1996 marked an inline comment as done.Jul 15 2020, 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.Jul 16 2020, 3:59 AM
This revision now requires changes to proceed.Jul 16 2020, 3:59 AM
ftynse added inline comments.Jul 16 2020, 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.Jul 16 2020, 7:27 AM

ftynse's comments addressed

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

Done

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

Thanks for the cleanup!

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

Changes evolved and merged from parent commit

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

Looks great, thanks!

This revision was automatically updated to reflect the committed changes.