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?