Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
please rebase against main
(since these files were extended)
mlir/include/mlir/Dialect/GPU/IR/GPUBase.td | ||
---|---|---|
114 | do we want to refine the "sparse handle type" strings as well for all four now? | |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
171 | move this up to other includes also, always double check that you don't need changes in bazel/cmake when you add a dependence | |
174 | for my understanding, why default? can we make e.g. Env default ;-) | |
181 | use same order as above, i.e. env first |
I have fixed this diff so that all tests are passed
mlir/include/mlir/Dialect/GPU/IR/GPUBase.td | ||
---|---|---|
114 | good catch! I added a few words here | |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
171 | Good catch! I tested and found it was not needed. I added that during the debugging process. | |
174 | That makes sense. I remove that | |
181 | Done |
mlir/lib/Dialect/SparseTensor/Transforms/SparseGPUCodegen.cpp | ||
---|---|---|
554 | update here and elsewhere in SpMM |
mlir/lib/Dialect/SparseTensor/Transforms/SparseGPUCodegen.cpp | ||
---|---|---|
554 | Got it! This is updated now |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
---|---|---|
189 | more modern casting? |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
---|---|---|
188 | intentional? |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
---|---|---|
189 | This custom classof is completely unnecessary. credit to Peiming and wrengr |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
---|---|---|
173–174 | You need to add the "typename" keyword after the equals sign. This will fix some of the build errors. (For the other build errors, be sure to rerun git clang-format HEAD^ and then upload the new version of the differential) |
Just a few style remarks, but other than that this is looking pretty good
mlir/include/mlir/Dialect/GPU/IR/GPUBase.td | ||
---|---|---|
116 | Could you reformat this section to:
def GPU_SparseDnVecHandle : DialectType<GPU_Dialect, CPred<"$_self.isa<::mlir::gpu::SparseDnVecHandleType>()">, "sparse dense vector handle type">, BuildableType<"mlir::gpu::SparseDnVecHandleType::get($_builder.getContext())">; | |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
1560–1561 | Nit: Here and below, can you reformat the results line to avoid going over 80 columns? Indentation should match the other let arguments = (ins ...); and let results = (outs ...); in this file | |
mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp | ||
1484–1485 | Why not define the helper template-function I suggested over chat, so as to avoid repeating so much code? (In case you forgot, that was: template<typename T> static void addOpaquePointerConversion(LLVMTypeConverter &converter) { converter.addConversion([&converter](T) -> Type { return converter.getPointerType( IntegerType::get(&converter.getContext(), 8)); }); } Then the definition of this function becomes just: void mlir::populateGpuToLLVMConversionPatterns(LLVMTypeConverter &converter, .....) { addOpaquePointerConversion<gpu::AsyncTokenType>(converter); addOpaquePointerConversion<gpu::SparseDnVecHandleType>(converter); addOpaquePointerConversion<gpu::SparseDnMatHandleType>(converter); ..... |
mlir/include/mlir/Dialect/GPU/IR/GPUBase.td | ||
---|---|---|
116 | Done | |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
173–174 | Now I know I need to check the build log. Thanks! | |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
1560–1561 | Worked on it | |
mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp | ||
1484–1485 | I forgot that. THank you for this! |
mlir/include/mlir/Dialect/GPU/IR/GPUBase.td | ||
---|---|---|
122 | nit: "sparse dense vector handle" reads a bit weird (I see why you picked this, but then :134 should really be "sparse sparse matrix handle" for consistency). I would just remove the "sparse" for DnVec and DnMat from the description string | |
134 | remove tailing "in sparse" | |
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h | ||
178 | this is nice! | |
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
150 | same order as in decl, so spmat last | |
222 | same order as decl please, so env/dnvec/dnmat/spmat | |
232–233 | it would probably be good to have a central place for this string (in the type), so that printing and parsing matches | |
238 | here too |
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
---|---|---|
232–233 | It turned out this is way too complicated and static variables needs to be defined in .cpp anyway. |
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp | ||
---|---|---|
232–233 | but can it be static? we are not in an anonymous namespace at this point? |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
---|---|---|
1705 | Here and belowShould be aligned as others change you made. |
This appears to have broken the Windows bot: https://lab.llvm.org/buildbot/#/builders/13/builds/35754/steps/6/logs/stdio
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Dialect\GPU\IR\GPUDialect.cpp(175) : error C2220: the following warning is treated as an error C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Dialect\GPU\IR\GPUDialect.cpp(175) : warning C4715: 'getSparseHandleKeyword': not all control paths return a value
@NathanielMcVicar Thank you for catching this! I am creating a new diff to fix this issue https://reviews.llvm.org/D151405
do we want to refine the "sparse handle type" strings as well for all four now?