Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_codegen_foreach.mlir | ||
---|---|---|
110 | Note how this sequence is printed in col-major order |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
---|---|---|
373 | Should we always lowering foreach? or only when RT is disabled? I think the operator is general enough to be used for conversion as well. |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
---|---|---|
346–362 | Can you use RewriterBase::MergeBlocks to avoid manually remap like this? |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
---|---|---|
307–311 | The style guide actually discourage the use of "auto" unless the type is obvious from the context (line 309) or it makes the code safer. |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
26 | structure Note that this also shows the danger of repeating comments. In general, google style says that if something is documented in the header, it does not need to be repeated in cc file. I would just pick a shorter "header" comment here to separate the parts | |
65 | period | |
138 | note that with an eye on more dim level types, I like to make these asserts very defensive. | |
240 | you can remove this one | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
29 | yeah, as you suggested yourself, you can merge these with the sparsetensor.h ones | |
35 | oh nice, I just did this, and renamed it to toOrigDim | |
49 | loop structure to (co-iterate) over sparse tensors? | |
52 | relies -> rely | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
334 | if we don't do this, do we break debugging path? If it is not expensive, shall we always do it, just to avoid inconsistent paths under prod/debug? | |
373 | Yeah, if it is there, we lower (right now when RT is enabled, we won't see it). |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
---|---|---|
299 | the foreach |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
---|---|---|
346–362 | Thanks! It does make the code simpler! |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
---|---|---|
30–31 | Emitter (one m in comment) | |
37 | Add a doc on what constructor expects (the list of tensors) | |
45 | period at end? | |
53 | period at end? | |
57 | I just talked with the IREE team and we may find a better separation of codegen concerns by migrating the vectorization and parallelization to a downstream pass. If it simplifies things for you know, I would omit the parallel/vector capabilities from these utils for now | |
73 | period at end? | |
119 | sparis -> sparsification | |
125 | for each tensor | |
129 | A lot of these are taken from the codegen structured (and sometimes renamed, which makes "recognition" a bit harder for me) E.g. / Sparse iteration information (by tensor and index). These arrays was useful to explain the vector of vector | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
314 | I like how this reads now! | |
334 | int64_t when dealing with rank? | |
345 | same |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
---|---|---|
129 | One thing that need to be mentioned is that all the variables here now are indexed by tensor+dim instead of tensor+loop idx. |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
74 | I like the idea of encforcing proper API usage through asserts and other means a lot. Same for methods below. | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
34 | Please document the class also. In general, showing "usage" is very illustrative. So you can give a very short pseudo-code example of starting entering exiting and leaving a loop using the API of the class. | |
55 | typo: generated | |
80 | This is not really a getter. Just give it its own doc stating that it will return the coordinate lists in the passed array | |
85 | this is a lot of code for a header, and it distracts from reading the API why not move this block into the cpp file? | |
109 | I am not sure I am a big fan of using two completely different data structures for debug/prod, since that has the risk of introducing bugs on one path and not the other. Can't we just simply always keep all the bookkeeping with proper detection of wrong usage? | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorRewriting.cpp | ||
333 | Actually, I would remove this comment completely. The API of the emitter requires every loop to be opened/closed properly (and asserting on this behavior is a good thing). There is really no good benefit telling the reader here that a "shortcut" usage is possible ;-) | |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_codegen_foreach.mlir | ||
2 | This seems a copy and paste error (kDirect only impacts sparse2sparse) | |
16 | Can we use the common names for this, DCSC etc. | |
23 | Use common comment format with capital/period at end | |
25 | such a nifty test for this construct! |
yeah, as you suggested yourself, you can merge these with the sparsetensor.h ones