Page MenuHomePhabricator

Add convenience C++ helper to manipulate ranked strided memref
ClosedPublic

Authored by mehdi_amini on Feb 5 2021, 5:49 PM.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 5 2021, 5:49 PM
mehdi_amini requested review of this revision.Feb 5 2021, 5:49 PM
bondhugula requested changes to this revision.Feb 6 2021, 10:51 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
81

Missing doc comment (for the field above as well btw). All fields should be documented. On another note, the doc comment on Vector1D should have been triple ///.

214

If Rank is 0, offset will become -1. Is this intended? A comment is missing at least here and above where you have int dim = Rank - 1. Otherwise, you need a static_assert on Rank not being zero. The offset field is also missing a doc comment.

236–238

Missing doc comments however obvious it may appear now.

mlir/include/mlir/ExecutionEngine/MemRefUtils.h
10

Nit: Missing period at the end.

41–44

Triple /// comments please.

47

Assert message please.

51

negatice -> negative

58

Triple /// here and everywhere below.

143

-> for ranked strided memrefs.

207

Missing doc comment.

214

Missing new line at end.

mlir/unittests/ExecutionEngine/Invoke.cpp
110

auto -> float

117–122

Test rank = 0 case as well? It may exercise corner cases.

This revision now requires changes to proceed.Feb 6 2021, 10:51 PM

Can you please add a commit summary as well - "manipulate ranked strided memref" doesn't capture all of this support accurately.

ftynse added inline comments.Feb 8 2021, 4:46 AM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
136

Would it make sense to accept any range rather than just an initializer list?

137

Nit: please add an assertion message

mlir/include/mlir/ExecutionEngine/MemRefUtils.h
80

Nit: "mallocs" sounds like there is heap allocation happening, but it is not the case here.

103

The other functions return the descriptor by-value rather than by-pointer. I suppose this template is not instantiated, otherwise it should have triggered an error.

110

s/allow/allows one/

113

I would suggest casting the result back to pair<T *, T *> before returning. This will also let makeStridedMemRefDescriptor functions take T * rather than void * for better type checking. Furthermore, if the template arguments in makeStridedMemRefDescriptor are swapped to template <int N, typename T>, I expect type deduction to kick in for element types.

mehdi_amini added inline comments.Feb 8 2021, 1:16 PM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
81

Yes, but note that I'm just doing a mechanical namespace change here. This whole file is a bit messy in my opinion, I'd like to revamp the whole directory in the near future actually.

136

As a template?

template<typename Range>
T &operator[](Range indices) {
 ...
mehdi_amini marked 18 inline comments as done.

Address comment and add more tests

Thanks for the review!

I added a bunch of more test to increase the coverage: rank-0, rank-1, and a test with a callback to a C++ native function.

PTAL!

mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
214

Added a specialization for rank 0, and tests for rank0 and rank1

bondhugula accepted this revision.Feb 8 2021, 11:30 PM

A few more minor comments. Please also address the commit summary comment above. This functionality is looking great to me. Please do go with Alex's deeper review comments.

mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
81

I see - missed that. Would be great to have a subsequent NFC patch.

195

indices.empty() please.

196

no -> number

mlir/unittests/ExecutionEngine/Invoke.cpp
248

Micro-nit: drop reference to elt - isn't it more overhead FWIW?

This revision is now accepted and ready to land.Feb 8 2021, 11:30 PM
mlir/include/mlir/ExecutionEngine/MemRefUtils.h
2

I am assuming you are also refactoring usage in iree/experimental/ModelBuilder, in a separate revision?

ftynse accepted this revision.Feb 9 2021, 1:21 AM
ftynse added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
136

Cool, thanks!

mehdi_amini added inline comments.Feb 9 2021, 4:53 PM
mlir/include/mlir/ExecutionEngine/MemRefUtils.h
2

What do you mean exactly? (I haven't touched anything IREE related so far)

mlir/unittests/ExecutionEngine/Invoke.cpp
248

Will do. It was a copy paste from the loop above where I assigned to elt

mehdi_amini marked 3 inline comments as done.Feb 10 2021, 9:39 AM
mehdi_amini added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
196

used "empty"

mehdi_amini marked an inline comment as done.Feb 10 2021, 9:40 AM
This revision was landed with ongoing or failed builds.Feb 10 2021, 9:41 AM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Feb 10 2021, 10:57 AM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
195

It does not work with initializer_list actually :(

bondhugula added inline comments.Feb 10 2021, 11:07 AM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
195

Oh, didn't realize that. Never mind.

mlir/include/mlir/ExecutionEngine/MemRefUtils.h
2

I mean that this file (https://github.com/google/iree/blob/main/experimental/ModelBuilder/MemRefUtils.h) should disappear and its uses updated now that you have moved and improved the functionality in core.

Please make sure we have only 1 source of truth for all this (yours).

mehdi_amini added inline comments.Feb 10 2021, 12:54 PM
mlir/include/mlir/ExecutionEngine/MemRefUtils.h
2

I'm a bit confused about how you're bringing downstream project management into MLIR core review here?
I may be missing something but it seems to me that it should be entirely independent: I see it as "up to IREE" to decide if they want to adopt new upstream capabilities as they see fit.
(and separately, it's a question for non-IREE google internal projects to switch to MLIR provided tools instead of IREE/experimental/...)

mlir/include/mlir/ExecutionEngine/MemRefUtils.h
2

When code gets moved around I feel it's only natural to gently ask for an update of the former source of truth.
Yes, ModelBuilder wants to use this improved version, can you please also update IREE separately to avoid what is now copy-pasta of core? Thanks much!