This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse][gpu] fix F32 bug for SpMV and SpMM
ClosedPublic

Authored by aartbik on May 23 2023, 1:49 PM.

Details

Summary

The alpha/beta variables, residing on the host, should have the
32-bit or 64-bit width of the result type. It was formerly always
passed as double.

Diff Detail

Event Timeline

aartbik created this revision.May 23 2023, 1:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
aartbik requested review of this revision.May 23 2023, 1:49 PM
Peiming added inline comments.May 23 2023, 1:55 PM
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp
251–260

This is indeed magic :-)

I would suggest change it to make it a little bit safer from name collision

aartbik added inline comments.May 23 2023, 2:22 PM
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp
251–260

You mean adding the variable names as parameter too?
Sure, that seems a good idea, perhaps I can even use the append feature for the derived names then....

Peiming added inline comments.May 23 2023, 2:34 PM
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp
251–260

Oh I mean adding extra braces around alphaf, etc

But adding the variable name as parameter might be a good idea too.

aartbik updated this revision to Diff 524907.May 23 2023, 3:24 PM

even more magic!

aartbik added inline comments.May 23 2023, 3:35 PM
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp
251–260

Oh, braces too. I think in that case, the locals go out of scope at the closing }
making the pointers invalid, right?

But see if you like the new magic better. It is really just to avoid repeating the same block of code over and over again, while keeping it local scope

Peiming added inline comments.May 23 2023, 4:04 PM
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp
251–260

Yeah, that's why I still keep alphap out of the scope (only variable ending with -f and -d are in local scope).

Peiming added inline comments.May 23 2023, 4:07 PM
mlir/lib/ExecutionEngine/CudaRuntimeWrappers.cpp
251–260

NVM, you are right ;-)

Peiming accepted this revision.May 23 2023, 4:07 PM
This revision is now accepted and ready to land.May 23 2023, 4:07 PM
This revision was automatically updated to reflect the committed changes.