Hey! Thanks for the patch! I wondered if it would make sense to move this utility to a public utility header. I found myself needing it locally and I wouldn't like to just copy-paste it. There is another topological sort for ops under 'include/mlir/Analysis/SliceAnalysis.h'. Maybe we could move them both to 'include/mlir/IR/Utils.h'? WDYT?

Thanks, Alex. Will do.

Thanks, LGTM! Please, give some time for @ftynse to have a look.

Thanks, Nicolas. The failure doesn't seem related to this:

Sorry, I misunderstood what an unranked memref models. This indeed doesn't make sense as is. Thanks.

• Removed support for unranked memrefs
• Addressed feedback

Thanks, Alex! Sorry, I misunderstood what an unranked memref is modeling. Let me remove that from the patch since it's incorrect and only leave the support for call and return ops. We can add support for unranked memrefs when needed.

Thanks!

Ping :)

As a general observation, do you folks actually want the bare-pointer convention, or is it only there to attach noalias argument attribute? Maybe we should consider investing into expressing aliasing information in MLIR rather than keep the maintenance burden.

• Refactor code to a utility function.
• Replace i64 type with index type.
• Address Alex's feedback.

Sorry for the delay and sorry, Nicolas, I had written the example that
you were asking for in the .cpp doc but I forgot to copy it to the header
file. This patch is doing that. Please, let me know if there are any other
comments. Thanks!

This commit seems to be breaking the build with -DBUILD_SHARED_LIBS=TRUE. Could you please have a quick look? Thanks!

Ping :)

LGTM! Just some nit comments! Thanks for contributing this feature!

Thanks! Any other comments, @aartbik?

Thanks! Any other comments? @aartbik?

Addressed feedback. Thanks!

Addressed feedback. Thanks!

In D86986#2251245, @bondhugula wrote:

I'd reword the commit summary a bit to replace "Introduce" - instead, something like "Make use of ..."

Thanks for the patch! I can confirm that it is fixing our build locally (using gcc 7.5.0).

Thanks! Addressing feedback.

Addressed all the comments and replaced the DFS loop container in the API with
an 'std::vector<SmallVector<AffineForOp, 2>>' (we use something similar in other
loop utilities). This container is a bit more friendly for a public API than
providing a plain list of loops in DFS order and also explicitly represents the
nesting level relationship of the loops that we want to vectorized, which is
useful for the vectorization algorithm itself. The vectorizer is therefore updated
to work on this container.

Thanks, Aart! Sorry for the delay. I've been OOO. I'll take a look at your comments and I'll also try to use 'std::vector<SmallVector<AffineForOp, 2>>' for the API so that we can preserve some kind of loop nest structure.

Thanks Frank! Sorry, I was OOO last week. Quick comments for now, I'll come back later.

Thanks, Alex!

Unblocking the review from my side. OOO next week.

Thanks for addressing the comments, Frank! LGTM. I'll leave the final approval to Uday and River since I'm OOO next week.

Thanks Frank! It looks good to me. Just a few minor comments

Thanks for the patch! Just a high-level comment for now. It looks good overall.

Thanks Jeremy! Some minor comments.

Thanks for fixing this! LGTM

Thanks Uday! Good point. I haven't looked too much into the slice computation but if you can send me some pointers and some examples on how the regions should be computed I could have a look. Thanks!

If no more comments, I'll land this after https://reviews.llvm.org/D81302