This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [gpu] [sparse] refined SparseHandle type
ClosedPublic

Authored by K-Wu on May 19 2023, 9:03 PM.
Tokens
"Love" token, awarded by K-Wu.

Diff Detail

Event Timeline

K-Wu created this revision.May 19 2023, 9:03 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
K-Wu requested review of this revision.May 19 2023, 9:03 PM

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

K-Wu updated this revision to Diff 524369.May 22 2023, 9:53 AM

upd gpubase.td

K-Wu updated this revision to Diff 524433.May 22 2023, 12:38 PM

upd class names in the commits rebase-pulled from origin/main

K-Wu updated this revision to Diff 524480.May 22 2023, 1:58 PM

fix broken tests

K-Wu marked 4 inline comments as done.May 22 2023, 2:00 PM

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

aartbik added inline comments.May 22 2023, 2:10 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseGPUCodegen.cpp
554

update here and elsewhere in SpMM

K-Wu updated this revision to Diff 524492.May 22 2023, 2:31 PM
K-Wu marked 4 inline comments as done.

update to refined sparse handles in incoming changes

K-Wu marked an inline comment as done.May 22 2023, 2:33 PM
K-Wu added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseGPUCodegen.cpp
554

Got it! This is updated now

K-Wu marked an inline comment as done.May 22 2023, 2:33 PM
aartbik added inline comments.May 22 2023, 4:42 PM
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h
189

more modern casting?

aartbik added inline comments.May 22 2023, 4:43 PM
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h
188

intentional?

K-Wu updated this revision to Diff 524536.May 22 2023, 5:07 PM

rm the unnecessary classof impl

K-Wu marked 2 inline comments as done.May 22 2023, 5:09 PM
K-Wu added inline comments.
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h
189

This custom classof is completely unnecessary. credit to Peiming and wrengr

K-Wu updated this revision to Diff 524537.May 22 2023, 5:18 PM
K-Wu marked an inline comment as done.

rm unnecessary code

K-Wu updated this revision to Diff 524543.May 22 2023, 5:32 PM

readability

wrengr added inline comments.May 22 2023, 6:48 PM
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:

  • Add a blank line between each definition
  • Reflow the code to avoid going over 80 columns. E.g., using the following format:
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);
  .....
K-Wu added a comment.May 22 2023, 7:42 PM

Just a few style remarks, but other than that this is looking pretty good

I will address these two comments tomorrow.

K-Wu updated this revision to Diff 524733.May 23 2023, 8:13 AM

reformat and changes to make sure build could pass

K-Wu updated this revision to Diff 524737.May 23 2023, 8:21 AM

add helper func as wrengr suggested

K-Wu awarded a token.May 23 2023, 8:24 AM
K-Wu marked 4 inline comments as done.May 23 2023, 8:26 AM
K-Wu added inline comments.
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!

K-Wu updated this revision to Diff 524753.May 23 2023, 9:04 AM
K-Wu marked 4 inline comments as done.

reformat

K-Wu updated this revision to Diff 524754.May 23 2023, 9:06 AM

fixe variable name issue when rebase-pulling

aartbik added inline comments.May 23 2023, 10:15 AM
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

215

same order as decl please, so env/dnvec/dnmat/spmat

225–226

it would probably be good to have a central place for this string (in the type), so that printing and parsing matches

231

here too

K-Wu updated this revision to Diff 524794.May 23 2023, 10:27 AM

addressing the order of refined HandleType

K-Wu marked 5 inline comments as done.May 23 2023, 10:32 AM
K-Wu added inline comments.
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h
178

credit to

mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
225–226

How about something like adding a static constexpr string in each HandleType definition, i.e., those template specialization lines?

K-Wu added inline comments.May 23 2023, 10:33 AM
mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h
178

Great thanks to @wrengr and @Peiming

K-Wu updated this revision to Diff 524876.May 23 2023, 1:57 PM

centralize keywords

K-Wu marked an inline comment as done.May 23 2023, 1:59 PM
K-Wu added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
225–226

It turned out this is way too complicated and static variables needs to be defined in .cpp anyway.
I am using std::map to centralize the keywords

aartbik accepted this revision.May 23 2023, 2:02 PM
aartbik added inline comments.
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
225–226

but can it be static? we are not in an anonymous namespace at this point?
or move it there?

This revision is now accepted and ready to land.May 23 2023, 2:02 PM
Peiming added inline comments.May 23 2023, 2:05 PM
mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
1705

Here and belowShould be aligned as others change you made.

K-Wu updated this revision to Diff 524878.May 23 2023, 2:07 PM

add static keyword

K-Wu updated this revision to Diff 524880.May 23 2023, 2:13 PM

formatted the td

K-Wu marked 2 inline comments as done.May 23 2023, 2:14 PM
K-Wu updated this revision to Diff 524885.May 23 2023, 2:24 PM

fix typo

K-Wu updated this revision to Diff 524896.May 23 2023, 2:58 PM

lazy eval to fix global destructor compiler warning

aartbik updated this revision to Diff 525230.May 24 2023, 9:44 AM

rebased with main

This revision was landed with ongoing or failed builds.May 24 2023, 10:16 AM
This revision was automatically updated to reflect the committed changes.

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
K-Wu added a comment.May 24 2023, 9:18 PM

@NathanielMcVicar Thank you for catching this! I am creating a new diff to fix this issue https://reviews.llvm.org/D151405