This is an archive of the discontinued LLVM Phabricator instance.

[mlir][NFC] Refactor ClassID into a TypeID class.
ClosedPublic

Authored by rriddle on Apr 8 2020, 6:31 PM.

Details

Summary

ClassID is a bit janky right now as it involves passing a magic pointer around. This revision hides the internal implementation mechanism within a new class TypeID. This class is a value-typed wrapper around the original ClassID implementation.

Diff Detail

Event Timeline

rriddle created this revision.Apr 8 2020, 6:31 PM

Seems like a big change. I don't know if it's worthwhile breaking it down first (like const ClassID * -> TypeID, as a separate change). Might be hard to get the const correctness right, I guess.

mlir/include/mlir/IR/Location.h
280

getTypeID()?

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
44–45 ↗(On Diff #256155)

rename seems unrelated?

mlir/lib/IR/Location.cpp
133

maybe getTypeID()?

rriddle updated this revision to Diff 256171.Apr 8 2020, 8:35 PM
rriddle marked 3 inline comments as done.

Resolve comments

Seems like a big change. I don't know if it's worthwhile breaking it down first (like const ClassID * -> TypeID, as a separate change). Might be hard to get the const correctness right, I guess.

Yeah, happy to split it. I mainly wanted one patch to make it easier for me to transfer to my windows machine.

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
44–45 ↗(On Diff #256155)

It's actually not, as the name of this pass matches the one for the other LICM.

mehdi_amini added inline comments.Apr 8 2020, 8:56 PM
mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
44–45 ↗(On Diff #256155)

That has me concerned with the mechanism here if it can't differentiate.

rriddle updated this revision to Diff 256183.Apr 8 2020, 9:53 PM

Remove magic string handling.

rriddle retitled this revision from [mlir] Refactor ClassID into a more robust TypeID class. to [mlir][NFC] Refactor ClassID into a TypeID class..Apr 8 2020, 9:54 PM
rriddle edited the summary of this revision. (Show Details)
rriddle updated this revision to Diff 256184.Apr 8 2020, 10:02 PM
rriddle marked an inline comment as done.

Update

mlir/lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp
44–45 ↗(On Diff #256155)

I discovered a magic incantation to get things to link properly across DLLs, but it requires a combo of dllexport/dllimport which LLVM doesn't even use now. We can punt until linking across DLLs becomes a problem. In the meantime this refactoring is still valuable.

mehdi_amini accepted this revision.Apr 10 2020, 8:52 PM
mehdi_amini added inline comments.
mlir/include/mlir/Support/TypeID.h
24

I would extend a bit the description with more info about the intended use and how it work.

This revision is now accepted and ready to land.Apr 10 2020, 8:52 PM
rriddle updated this revision to Diff 256748.Apr 10 2020, 11:43 PM

Resolve comments. Add more documentation.

rriddle marked an inline comment as done.Apr 10 2020, 11:46 PM
This revision was automatically updated to reflect the committed changes.