Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
Can you please add a commit summary as well - "manipulate ranked strided memref" doesn't capture all of this support accurately.
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. |
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) { ... |
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 |
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? |
mlir/include/mlir/ExecutionEngine/MemRefUtils.h | ||
---|---|---|
1 | I am assuming you are also refactoring usage in iree/experimental/ModelBuilder, in a separate revision? |
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h | ||
---|---|---|
136 | Cool, thanks! |
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h | ||
---|---|---|
196 | used "empty" |
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h | ||
---|---|---|
195 | It does not work with initializer_list actually :( |
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h | ||
---|---|---|
195 | Oh, didn't realize that. Never mind. |
mlir/include/mlir/ExecutionEngine/MemRefUtils.h | ||
---|---|---|
1 | 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). |
mlir/include/mlir/ExecutionEngine/MemRefUtils.h | ||
---|---|---|
1 | I'm a bit confused about how you're bringing downstream project management into MLIR core review here? |
mlir/include/mlir/ExecutionEngine/MemRefUtils.h | ||
---|---|---|
1 | When code gets moved around I feel it's only natural to gently ask for an update of the former source of truth. |
mlir/include/mlir/ExecutionEngine/MemRefUtils.h | ||
---|---|---|
195 | @mehdi_amini incorrect argument order for memset memset(&other.descriptor, 0, sizeof(other.descriptor)); Reported here https://pvs-studio.com/en/blog/posts/cpp/1003/ (N10) |
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 ///.