Details

Reviewers

aartbik
bixia
Peiming
nicolasvasilache

Commits

rG74c54206d7ab: [mlir][sparse] moving kInvalidId into "detail" namespace

Summary

In the next few commits I will be converting the various Merger identifier typedefs into newtypes; and once that's done, the kInvalidId constant will only be used internally and therefore does not need to be part of the public mlir::sparse_tensor namespace.

Depends On D146673

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

wrengr created this revision.Mar 22 2023, 5:13 PM

Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 5:13 PM

Herald added subscribers: hanchung, jsetoain, Moerafaat and 20 others. · View Herald Transcript

wrengr requested review of this revision.Mar 22 2023, 5:13 PM

Herald added a reviewer: nicolasvasilache. · View Herald TranscriptMar 22 2023, 5:13 PM

Herald added a project: Restricted Project. · View Herald Transcript

Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript

Harbormaster completed remote builds in B221177: Diff 507557.Mar 22 2023, 5:14 PM

wrengr added a child revision: D146676: [mlir][sparse] Factoring out LoopEmitter::isValidLevel.Mar 22 2023, 5:14 PM

aartbik accepted this revision.Mar 22 2023, 6:33 PM

aartbik added inline comments.

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1114	are you deliberately enforcing the ::mlir top level instead of relying on the using mlir::sparse_tensor with the shorter version of this? (I assume yes, but just checking)

This revision is now accepted and ready to land.Mar 22 2023, 6:33 PM

wrengr added inline comments.Mar 22 2023, 6:41 PM

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
1114	yeah it's deliberate, just for paranoia's sake really. I'm pretty sure just using `sparse_tensor::detail::kInvalidId` would work too. But in the next few CLs this'll be replaced with `if (!e.isValid())` so I'm not woo worried about the verbiage in the interim