This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Refactoring: remove Operation * from the argument list in utility functions
ClosedPublic

Authored by Peiming on Aug 16 2022, 1:52 PM.

Details

Summary

This patch remove the Operation *op from the argument list in utility functions, and directly pass the Location instead of calling op->getLoc().

This should make the code more clear, as the utility function (logically) does not relies on the operation that we are currently rewriting, and they behave the same regardless of the operation.

Diff Detail

Event Timeline

Peiming created this revision.Aug 16 2022, 1:52 PM
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.Aug 16 2022, 1:52 PM
Peiming edited the summary of this revision. (Show Details)Aug 16 2022, 1:54 PM
Peiming added reviewers: wrengr, bixia, anlunx.
Peiming updated this revision to Diff 453121.Aug 16 2022, 1:56 PM

Remove out-dated comments

aartbik added inline comments.Aug 16 2022, 2:07 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
80–82

remove this, but extend L74 to say "Creates .... in builder's module"

Peiming updated this revision to Diff 453125.Aug 16 2022, 2:09 PM

address comments

Peiming marked an inline comment as done.Aug 16 2022, 2:09 PM
wrengr accepted this revision.Aug 16 2022, 2:14 PM

One minor style nit: if you're inlining the TypeRange noTp; then it'd be cleaner to say {} rather than TypeRange(). The former will desugar to the latter, so it's not a big deal, just a bit more succinct is all.

This revision is now accepted and ready to land.Aug 16 2022, 2:14 PM
aartbik accepted this revision.Aug 16 2022, 2:16 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
64

This is of course less precise then the op's loc, but I am not sure how much effort we really put in keeping locations very up to date

Peiming updated this revision to Diff 453128.Aug 16 2022, 2:18 PM

Address comments

Peiming added inline comments.Aug 16 2022, 2:21 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
64

I agree, but it is not the call instruction, there is no actual location for the runtime interface anyway.

Also, say, if there are multiple operations that use the same function, the location will always be the first operation that creates it, which does not make sense either, while the module location is consistent at least.

Peiming added inline comments.Aug 16 2022, 2:25 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
82

@aartbik This is also why I used the loc from op->getLoc() for CallOp here instead of passing it to getFunc

This revision was landed with ongoing or failed builds.Aug 16 2022, 2:27 PM
This revision was automatically updated to reflect the committed changes.