Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir | ||
|---|---|---|
| 5 | I may be mistaken, but doesn't mlir_runner_utils link in mlir_c_runner_util as well? can you please check? | |
| 7 | can you please add an empty line after the RUN section to be consistent with the other tests | |
| 14 | just indent by 2 | |
| 32 | perhaps a few comments on what you test here | |
| 42 | indent by 2 | |
| mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir | ||
|---|---|---|
| 5 | Indeed. Removed the redundant mlir_c_runner_util. | |
| mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir | ||
|---|---|---|
| 13 | Note that the caller never deallocs this explicitly, perhaps add this too for pedantic accuracy | |
Thanks for the quick review, Aart!
| mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir | ||
|---|---|---|
| 14 | Added the deallocs | |
LG thanks @ThomasRaoux
| mlir/integration_test/Dialect/Vector/CPU/test-vector-distribute.mlir | ||
|---|---|---|
| 13 | With the tensor constant suggestion, the alloc / dealloc would be properly inserted. | |
| 32 | typo | |
| 41 | With https://reviews.llvm.org/D88998 we can just create a tensor constant and lower it to memref. If you reduced that to say 5, the test could become tighter and more semantically meaningful. OTOH there would still be a need for extracting the data from a tensor to a memref, so maybe this is not worth it and some abstractions are still missing. Feel free to ignore, just wanted to surface the fact that the option exists now. | |
I may be mistaken, but doesn't mlir_runner_utils link in mlir_c_runner_util as well? can you please check?