Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
stuck at declaring tuple literal in .mlir
mlir/test/Conversion/GPUCommon/2To4Sparsity/lower-2to4-sparse-to-gpu-runtime-calls.mlir | ||
---|---|---|
27 ↗ | (On Diff #527667) | How can I create a three-element tuple literal here? |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
---|---|---|
1675 | why did you add the handle to DnMat? | |
2006 | this change seems unrelated to the core change of this revision? | |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
193–194 | Since you change the logic of the make I would move these two "find_lib" below to L217 "we need" as we did for L190 but now say, "Determine if cuSPARSE lib and cuSPARSELT are available and, if so, set the MLIR_CUDA_SM80_SPARSE_ENABLED flag" | |
216 | so right before this, the two find_lib without REQUIRED now | |
233 | We now have situations like (1) we cannot find either lib do we simply want to ONLY support (3) and guard all code with the MLIR_CUDA_SM80_SPARSE_ENABLED ? | |
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp | ||
24–28 | since we now have builds possible without cusparse lib, don't we need a macro for this (and all cusparse wrappers? | |
447 | I would move this to L438, and use two empty lines around the #if since it applies to rest of file | |
592 | newline? |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
---|---|---|
233 | Working on it. Adding a second flag in the wrapper cpp where CUSPARSE disabled will imply CUSPARSELT disabled |
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp | ||
---|---|---|
447 | addressed. Let me know if your thought is different from my new changes :D |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
---|---|---|
193–194 | Sorry I failed to sync changes from another machine which I thought had been synced by arcanist. Please check the following few lines for the improved CMake build flag logic. Please also check if the runtime flags in the CUDARuntimeWrappers.cpp address the comments. |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
---|---|---|
1675 | cusparseLt's API is a bit different from cusparse and requires environment handle as input. But I am open to ideas to split the revision. Also it seems feasible to retrieve the env implicitly as well. Let's discuss offline maybe. | |
2006 | When using cusparseLt I get three sizes when invoking spmm_buffer_size. THe three sizes are for workspace, compressed data and compressed buffer. Tablegen asks for a type in the assembly to understand whether the output is one size, or a tuple of three sizes. Let us discuss offline how to improve this. |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
---|---|---|
1675 | Ah for the 2:4 case. shall we add it to dn_vec too then, for consistency? | |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
194–195 | I gave this some more thought, and I don't think we have any precedent in LLVM/MLIR building conditionally on the context. In fact, some users may not like this, since exactly the same config will result in different binaries on different installations, causing perhaps hard-to-track bugs on bots. So let's keep the two flags, but keep the build logic if(MLIR_ENABLE_CUDA_CUSPARSE) find_library(CUDA_CUSPARSE_LIBRARY cusparse HINTS ${CMAKE_CUDA_IMPLICIT_LINK_DIRECTORIES} REQUIRED) ... if(MLIR_ENABLE_CUDA_CUSPARSELT) find_library(CUDA_CUSPARSELT_LIBRARY cusparseLt HINTS ${CMAKE_CUDA_IMPLICIT_LINK_DIRECTORIES} REQUIRED ) find_path(CUDA_CUSPARSELT_HEADER cusparseLt.h HINTS ${CMAKE_CUDA_TOOLKIT_INCLUDE_DIRECTORIES} REQUIRED) ... Note |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
---|---|---|
194–195 | That makes sense. Yeah cusparseLt uses some data structures in cusparse so (2) is very reasonable |
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td | ||
---|---|---|
1675 | That makes sense. Done. |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
---|---|---|
194–195 | This must now go under the CUDA_CUSPARSE_LIBRARY below otherwise, this line will fail without lib even when macro is not set | |
197 | Try to keep the comments under 80-col by breaking the line typo: installed Also, rephrase. Enabling the MLIR_ENABLE_CUDA_CUSPARSELT flag assumes the library is installed ... | |
198 | you don't want to nest this in the one below? |
mlir/lib/ExecutionEngine/CMakeLists.txt | ||
---|---|---|
216 | This comment is outdated, since we no longer set the flag
| |
224 | Find the libcusparseLt.so library if CUSPARSELT build is requested.<then rest of your comment, but broken up with 80-cols> | |
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp | ||
27 | I would nest this done inside CUSPARSE #if/#endif | |
443 | move this end to end of file, so we have nested definitions | |
mlir/test/Conversion/GPUCommon/2To4Sparsity/lit.local.cfg | ||
1 ↗ | (On Diff #528994) | not needed, we are not running this, just lowering |
mlir/test/Conversion/GPUCommon/2To4Sparsity/lower-2to4-sparse-to-gpu-runtime-calls.mlir | ||
1 ↗ | (On Diff #528994) | don't introduce a new 2To4Sparsity directory, simply move this test file up to the same level as e.g. lower-sparse-to-gpu-runtime-calls.mlir |
did you forget to add the lowering file to this revision after moving up?
mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp | ||
---|---|---|
818 | Also add CreataCooAoS for completeness? | |
822 | Start comments with upper case and end with period // Print the SpMat defining op. Here and below a few times | |
827 | a bit strange to return this as string? |
why did you add the handle to DnMat?