- User Since
- Dec 14 2019, 8:54 PM (34 w, 5 d)
Fix yet another inadvertent change.
Fix inadvertent change.
Combine match and rewrite; update LLVM lowering style.
Thanks for addressing everything.
Wed, Aug 12
I'll defer to the original reviewers on the list for a real review!
Please add a TODO somewhere on the dim op handling part.
Looks good - thanks.
Tue, Aug 11
Documentation and comments are missing throughout.
Mon, Aug 10
Looks great. Some initial comments.
Sat, Aug 8
Some minor fixes.
Fri, Aug 7
The functionality added looks great! Thanks. I'll shortly submit a review.
Looks good, thanks.
Thu, Aug 6
@AlexEichenberger Would you be able to review this - esp. for the part that's connected to what you wanted?
Looks great - let me know if you'd like me to commit.
Please add an [MLIR] tag to the commit title.
Looks great overall - ideally, I'd like to some more test case patterns - the additional tests can be small. Consider this one:
Wed, Aug 5
Thanks very much for this finally - this was long overdue. I skimmed through it - I think it's documenting most of the things. But I think it's also important to have a few lines where appropriate on what one can't do inside a pattern rewrite: in fact, this is typically what new users get wrong and source of many questions.
Tue, Aug 4
This is quite useful - thanks. Some minor comments on the first half.
Thanks for spotting this @dcaballe. That was clearly an incorrect assumption in the simplification being done for the all zero offset case which was thought to not require a remapping at all.
Sat, Aug 1
Consider changing title to something like '[MLIR] Added tiling validity check to loop tiling pass'. You'll have to hit "Edit revision" manually here as arc doesn't automatically update it.
I think the commit summary is more detailed / has additional info that could be in the doc / implementation comment of checkTilingValidity. You could move a few things from the former to the latter.
Thanks for revising this - looks good. Some minor comments and this one below.
Thanks for revising. Looks good to me - minor comments.
Fri, Jul 31
I notice auto being used pervasively and it's impacting readability at most places. Please spell the type out.
Thu, Jul 30
But no property for the expressions with different operations ((expr1 ceildiv expr2) floordiv expr3)
Thanks for implementing this functionality. Some superfluous comments on documentation to start with.
Please expand the commit summary a little. The h in h^T . R here are always the canonical directions with the current tiling pass. You really don't need to do anything with FlatAffineConstraints. Instead, the conditions of Iriogoin and Triolet would just simplify here to checking whether all dependence components are non-negative along the dimensions you want to tile. And checking this by simply looking at the dependence information is sufficient. You may want to expand on the motivation if you intended something else.
Can you please rephrase the commit title to avoid the name and instead just have it in natural language?