Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
553 | I think you will need to expand this comment with more details on what an "dependent" slice is exactly. |
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
443 | Please use the TensorId and LoopId types. Although they're just typedefs for now, by the end of the week I'll have a CL that changes them to be actual types. | |
443 | Elsewhere in this file we use the name "i" for loop identifiers, so please stick with that for consistency. | |
445 | Please use the Level type. Again, although it's just a typedef for now, I'll be changing that soon. | |
446 | What does "idx" mean here? I went through a lot of work cleaning up all the ambiguity about several different meanings of "index", so please use a non-ambiguous name that matches how things are named elsewhere | |
462 | should be "i" | |
469 | should be "i" | |
475 | elsewhere in this file we spell out "lvl" in order to avoid any potential confusion with loop identifiers, so you should stick with that here. | |
480 | ditto | |
486 | Based on the body of this method, that should actually be a TensorLoopId | |
558 | Please clarify what exactly this type is supposed to be. | |
558 | I've avoided using the name "ldx" in this file because it's too confusing. Please change this to "loop" or similar, to match the names used by the other fields | |
563 | please rename to "lvlToDependantLoop" or similar. (We never use "tlvl" anywhere. "depended" is misspelled. And I've intentionally avoided using "ldx" elsewhere in this file) | |
563 | Please clarify exactly what this type is supposed to be |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
770–771 | please clarify exactly what all these unsigned are actually supposed to be. | |
798 | Level | |
800 | LoopId | |
822 | LoopId | |
868 | Please clarify exactly what this type is supposed to be. Is it Dimension or something else? | |
876 | ditto | |
880 | ditto | |
895 | ditto | |
922 | ditto | |
927 | ditto | |
1870 | ditto | |
1871 | ditto | |
mlir/lib/Dialect/SparseTensor/Utils/Merger.cpp | ||
224 | ditto | |
226 | ditto | |
227 | ditto |
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
473 | list of loop indices (you use plural when saying something like set of XXXs) | |
554 | set of (tensor, level) pairs. | |
555 | expressions | |
557 | can you break this like ... that i and j are used ... | |
559 | dependent | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
113 | this comment does not match the implementation what you describe seems to match intended use elsewhere? | |
265 | Can you give a small example of that goes in and out here; just giving a basic example will go a long way in helping the reader | |
830 | "relax" is a bit easier to read (I think), same for function name | |
866 | \in just in (LaTeX ?) | |
939 | For this revision, can you not change the comment of this method / Computes a topologically sorted iteration graph for the linalg operation. so that the DIFF will be easier to line up the method with the original code? |
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
443 | If you're factoring this out, then you might as well factor out the getLvl(b) and getDimLevelType(b) expressions too... Originally I was going to call that a minor nit, but on reflection I've noticed that the current implementation allows the first callback to mutate those fields (via setLevelAndType) before the second callback. That's an extremely bug-prone design, so I can't imagine that it's intentional. But if for some reason it is intentional, then it needs to be explicitly documented that this is expected behavior and why it's intended. | |
445 | "level" | |
447 | "level" | |
461 | This should also be "i" | |
464 | Please rename this to loopToDependencies. Again, I do not use "ldx" anywhere in this file. | |
465 | Please rename this to levelToDependentLoops. Again, I've done a lot of work to avoid the ambiguous use of "idx". | |
468 | "loop" | |
468 | ditto my other comment about using "///" | |
468 | Should add a verb here (e.g., "Checks whether"). | |
473 | "which appear" | |
473 | Although this matches the name of the getMatchingIndexingMap method, I think it would be a lot clearer to call these "subscript expressions". | |
473–474 | ditto my other comment about using "///" | |
475 | Should be "dependent", here and everywhere else in this file. | |
479 | "lvl" | |
479 | When providing documentation this must be "///" so that it's picked up by the program that generates the documentation. | |
480 | This doesn't match the documentation. If the documentation is correct then this should be LoopId i. Otherwise, the documentation needs updating. | |
480 | "Lvl" | |
484 | This is inconsistent with the rest of the file. A LatPoint contains a whole set of TensorLoopIds. (If what is currently called TensorLoopId is actually supposed to be a "lattice point", then we need to rename the LatPoint and LatPointId types to something else to avoid confusion. In addition to updating all the documentation, method names, etc.) | |
484 | Ditto, although this matches the name of the getMatchingIndexingMap method, it would be clearer to call these "subscript expressions" throughout this file | |
486 | Should rename this to hasDependencies. (Because: this has nothing to do with "levels" so far as I can see. Calling it hasDependencies matches the name of the field it's querying. And it's always the case that "dependencies" correspond with "non-trivial subscripting expressions", so there's no need to spell out the latter in the method name.) | |
553 | "identifier" | |
554 | "identifier" | |
555 | "subscript" | |
557 | "subscript" | |
560 | "identifier" | |
562 | "its" | |
565 | "identifier" | |
565 | "lvl" |
fix issue
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
443 | OMG, you are SO right, i forget the else! |
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h | ||
---|---|---|
437–438 | please document the parameters of the expected callback (since the bool is no longer "trivial") | |
451 | in the description of the callback above, use names, and then say /*name=*/false here and above for true, so that it becomes more readable | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenEnv.h | ||
102 | hmm, I actually liked that I was able to hide the topsort completely in this new datastructure, but now it is leaking again but we have plans for an iteration graph class, then it would make sense perhaps to hide it there.. | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
277 | also, not now but perhaps later, if feels like all the affine analysis could also be in its own class, or perhaps with a new iteration graph class | |
399–406 | this method also does a lot more analysis than before, perhaps update the method doc for now a bit to reflect this | |
699 | typo | |
1815 | yeah, avoiding too many special cases, but making the general case and special case more uniform would be nice in the longer run |
please document the parameters of the expected callback (since the bool is no longer "trivial")