This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a new AttrTypeReplacer class to simplify sub element replacements
ClosedPublic

Authored by rriddle on Nov 9 2022, 9:25 PM.

Details

Summary

We currently only have the SubElement interface API for attribute/type
replacement, but this suffers from several issues; namely that it doesn't
allow caching across multiple replacements (very common), and also
creates a somewhat awkward/limited API. The new AttrTypeReplacer class
allows for registering replacements using a much cleaner API, similarly to
the TypeConverter class, removes a lot of manual interaction with the
sub element interfaces, and also better enables large scale replacements.

Diff Detail

Event Timeline

rriddle created this revision.Nov 9 2022, 9:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 9:25 PM
rriddle requested review of this revision.Nov 9 2022, 9:25 PM
Mogball added inline comments.Nov 10 2022, 9:21 AM
mlir/include/mlir/IR/SubElementInterfaces.h
36–38

🤔

71

I would like to file a feature request that walkSubElements etc get the same ability to specify WalkResult

mlir/lib/IR/SubElementInterfaces.cpp
116
mlir/lib/IR/SymbolTable.cpp
880

Should visiting locations be optional? Now all replacements will have to walk nested locations, but the vast majority of use cases don't want that (because Locations only recently got SubElementInterface implementations).

rriddle updated this revision to Diff 474590.Nov 10 2022, 12:17 PM
rriddle edited the summary of this revision. (Show Details)
rriddle marked 2 inline comments as done.
rriddle added inline comments.Nov 10 2022, 12:17 PM
mlir/include/mlir/IR/SubElementInterfaces.h
36–38

We've got some use cases that want to replace attributes/etc within types during a conversion pass. Right now we have some chained together lambdas that work together for the replacement(manually interacting through sub element interfaces), but having something with a better API makes it much easier.

71

I'll look at revamping the walk stuff in a followup!

mlir/lib/IR/SubElementInterfaces.cpp
116

Nice catch!

mlir/lib/IR/SymbolTable.cpp
880

Yeah, I can make it optional for now. I ran into some cases that broke when I stared putting interesting stuff in locations, but we can see how widely that applies before making this the default.

rriddle updated this revision to Diff 474591.Nov 10 2022, 12:17 PM
Mogball accepted this revision.Nov 11 2022, 4:54 PM
This revision is now accepted and ready to land.Nov 11 2022, 4:54 PM