Page MenuHomePhabricator

[mlir][sparse] Improving ExecutionEngine/SparseTensorUtils.h
ClosedPublic

Authored by wrengr on May 20 2022, 4:38 PM.

Details

Summary

This change makes the public API of SparseTensorUtils.cpp explicit, whereas before the publicity of these functions was only implicit. Implicit publicity is sufficient for mlir-opt to generate calls to these functions, but it's not enough to enable C/C++ code to call them directly in the usual way (i.e., without going through codegen). Thus, leaving the publicity implicit prevents development of other tools (e.g., microbenchmarks).

In addition this change also marks the functions MLIR_CRUNNERUTILS_EXPORT, which is required by the JIT under certain configurations (albeit not for anything in our test suite).

Diff Detail

Event Timeline

wrengr created this revision.May 20 2022, 4:38 PM
wrengr requested review of this revision.May 20 2022, 4:38 PM
aartbik added inline comments.May 24 2022, 9:06 AM
mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
17–18

is this still needed now?

1482

looks like we miss the 8/64 case too

1564

strange, that usually is the case when a { or ( is not closed properly (as far as the much more context insensitive lint is concernced).
try to match closing brackets to see if you find something

wrengr updated this revision to Diff 432359.Thu, May 26, 12:32 PM
wrengr marked 3 inline comments as done.

addressing comments

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
17–18

Nope. I'll also remove the <complex>, <cinttypes>, and <vector> includes, since they're also no longer necessary

1482

Bah, my random generator didn't catch that one ;) Also 32/64

I'll have to figure out a nice way to chain together x-macros to alleviate all the CASE and CASE_SECSAME situations. (Assuming I don't get things switched over to codegen first)

1564

After some further investigation, apparently the static keyword on the macro call for C32ABI somehow confuses clang-format and it assumes everything between the keyword and the next { or ; token must be part of a prototype. Luckily C++11 allows top-level extraneous semicolons (unlike earlier C++), so it's easy enough to resolve.

aartbik accepted this revision.Thu, May 26, 4:48 PM
This revision is now accepted and ready to land.Thu, May 26, 4:48 PM