This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Start migrating more dialects to prefixed form
ClosedPublic

Authored by jpienaar on Jun 16 2022, 9:22 PM.

Details

Summary

Marked all dialects that could be (reasonably) easily flipped to _Both
prefix. Updating the accessors to prefixed form will happen in follow
up, this was to flush out conflicts and to mark all dialects explicitly
as I plan to flip OpBase default to _Prefixed to avoid needing to
migrate new dialects.

Except for Standalone example which got flipped to _Prefixed.

Diff Detail

Event Timeline

jpienaar created this revision.Jun 16 2022, 9:22 PM
jpienaar requested review of this revision.Jun 16 2022, 9:22 PM
Mogball accepted this revision.Jun 16 2022, 9:26 PM

It begins

mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td
924

These should really just be deleted.

This revision is now accepted and ready to land.Jun 16 2022, 9:26 PM
jsetoain added inline comments.Jun 16 2022, 11:11 PM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
197–198

Should this change be part of this patch? Looks unrelated to all the other changes.

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
689–690

This doesn't look like it belongs in this patch either.

mlir/lib/Dialect/Tensor/Transforms/BufferizableOpInterfaceImpl.cpp
476–477

Same here. If the purpose is to add type prefixes to accessor methods, this doesn't look like part of that.

Mogball added inline comments.Jun 17 2022, 9:55 AM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
197–198

Some of the member functions now do different things because they were hidden before. E.g. getBody now regions the region body and hides the auto-generated function getBody for ops with single block regions

jpienaar marked 2 inline comments as done.Jun 18 2022, 9:36 AM
jpienaar added inline comments.
mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td
924

Agreed, I kept it mechanical so that folks can just do regex as part of this. I think we should improve this generically rather than these, but I'll leave them as is for now and make it easy to flip. But then yes we should do something better here, these are inconsistently added and feels out of place (we have the variant already that requires a context, perhaps that's an alternative to conciliate around that).

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
197–198

Yes, so Jeff explained the getBody instances. And here the type of steps change to match getSteps (as these are now the same function and getSteps is actually the convenience accessor with the type most used). If you look inside getSteps it's doing the casting internally that was done here. With Mehdi's upcoming change we could simplify this more.

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
689–690

It does as Jeff mentioned above.

This revision was landed with ongoing or failed builds.Jun 18 2022, 10:10 AM
This revision was automatically updated to reflect the committed changes.
jpienaar marked an inline comment as done.