This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Preliminary code changes for ExprId, LatPointId, LatSetId newtypes
ClosedPublic

Authored by wrengr on Mar 22 2023, 8:48 PM.

Details

Summary

This commit contains several code changes which are ultimately required for converting the varions Merger identifiers from typedefs to newtypes. The actual implementation of the newtypes themselves has been split off into separate commits, in hopes of simplifying the review process.

Depends On D146561

Diff Detail

Event Timeline

wrengr created this revision.Mar 22 2023, 8:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 8:48 PM
wrengr requested review of this revision.Mar 22 2023, 8:48 PM
wrengr edited the summary of this revision. (Show Details)Mar 22 2023, 9:55 PM
wrengr updated this revision to Diff 507601.Mar 22 2023, 9:56 PM

rebase & git-clang-format

aartbik added inline comments.Mar 29 2023, 10:06 AM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
31

Can you leave this in the same place for now (and not introduce a new MergerNewtypes.h). I am not convinced yes we really need two headers in the end, and even if so, we can have an intermediate revision that just moves this out. That way, I can focus on the real changes in the CL.

wrengr added inline comments.Mar 29 2023, 12:17 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
31

I think having a separate header makes a lot of sense, since by the end (i.e., after D146693) that header comes to ~500 lines of code, all of which is boilerplate stuff that distracts from "the 'real' API" presented in the Merger header. (Fwiw, I did start on some templating stuff to try to reduce the amount of boilerplate; but that's shelved for the time being, since I was running into issues getting the public/private/friend access controls to work right.)

Though I can postpone and split up the move, so that:

  • D146690 moves the ExprId, LatPointId, LatSetId types over (i.e., the same CL that converts those typedefs into newtypes)
  • D146691 moves the TensorId, LoopId, TensorLoopId types over (to avoid cyclic dependency issues)

Does that work for you?

aartbik added inline comments.Mar 29 2023, 12:23 PM
mlir/include/mlir/Dialect/SparseTensor/Utils/Merger.h
31

Yeah, postponing the split works for me, so that I can decide in the other revision if it is really worth the code size explosion.

But the changes in this revision all look reasonable, so for the sake of quick progress, leaving the typedefs in header for now will speed up this review.

wrengr updated this revision to Diff 509496.Mar 29 2023, 4:31 PM

Postponing the introduction of MergerNewtypes.h until D146690 (and D146691, D146693)

wrengr marked 2 inline comments as done.Mar 29 2023, 4:47 PM
aartbik accepted this revision.Mar 29 2023, 5:56 PM
This revision is now accepted and ready to land.Mar 29 2023, 5:56 PM
This revision was landed with ongoing or failed builds.Mar 29 2023, 6:02 PM
This revision was automatically updated to reflect the committed changes.