This is an archive of the discontinued LLVM Phabricator instance.

[mlir][NFC] Split OpBase.td into multiple smaller units
ClosedPublic

Authored by Groverkss on Jul 23 2023, 4:16 PM.

Details

Summary

This patch splits OpBase.td into smaller files, focusing on individual,
implementations of functionalities.

This patch is marked NFC, as it just splits the implementation into multiple
files and includes them back into OpBase.td, so it doesn't break anything.

Most of the patch is mechanical, with chunks of implementation being shifted
into indvidual files. The main things to look out are:

  • Classes and Definations added to Utils.td
  • Headers of files
  • Any thing that should have been split but isn't

Diff Detail

Event Timeline

Groverkss created this revision.Jul 23 2023, 4:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
Groverkss requested review of this revision.Jul 23 2023, 4:16 PM
Mogball accepted this revision.Jul 23 2023, 4:18 PM

awesome

This revision is now accepted and ready to land.Jul 23 2023, 4:18 PM
rriddle requested changes to this revision.Jul 23 2023, 4:32 PM
rriddle added inline comments.
mlir/include/mlir/IR/AttrTypeBase.td
1613

Quite a bit of this doesn't make sense to me, why are regions in the AttrType file? It'd also be nice to move all of the specialized attr/type constraints to a different file (so this one can just be focused on defining attributes/types).

This revision now requires changes to proceed.Jul 23 2023, 4:32 PM

A lot of the additions to this file shift away from the organization of the existing files. For example, AttrTypeBase.td is intended for defining attributes/types, not everything related to them. The idea of this review is amazing (and something I really appreciate and have been wanting for a while), but I think some of the organization here still needs to be worked out (e.g. I'd still expect things like Variadic/regions/etc to all be in OpBase, given they are related to op definitions not those for attributes/types).

Groverkss added inline comments.Jul 23 2023, 4:34 PM
mlir/include/mlir/IR/AttrTypeBase.td
1613

Ah, I missed the Region definitions here, sorry. I'll split the regions and the specialized constraints out.

Groverkss updated this revision to Diff 543338.Jul 23 2023, 5:06 PM
  • Move Region definations to OpBase.td
  • Move Primitive Properties to Properties.td
Groverkss updated this revision to Diff 543339.Jul 23 2023, 5:27 PM
  • Move OpTraits back to OpBase
  • Move specialized type constraints and attr constraints to a seperate file
Groverkss marked an inline comment as done.Jul 23 2023, 5:29 PM

@rriddle I think I was able to address your comments. Could you have a look again to see if you find anything that needs to be changed? Changes again, are mostly mechanical, with the commit messages explaining each small change.

Some other decisions that could use input:

I currently put the Attr, Type definitions in the "CommonType/AttrConstraints" files. Maybe the base definitions could live in Constraints.td, while the other things continue to live in Common... files?

Nice change! This LGTM overall, but let's wait for River to have another look

rriddle accepted this revision.Jul 25 2023, 12:18 AM

Nice!

mlir/include/mlir/IR/Constraints.td
186

Why are these not in CommonTypeConstraints?

mlir/include/mlir/IR/Interfaces.td
192

Drop the extra newline?

mlir/include/mlir/IR/Utils.td
64–67

Can these move to OpBase? I don't think they are that general.

This revision is now accepted and ready to land.Jul 25 2023, 12:18 AM
Groverkss marked 3 inline comments as done.
  • Address comments
mlir/include/mlir/IR/Constraints.td
186

I agree they make more sense there. Moved to CommonTypeConstraints.

mlir/include/mlir/IR/Utils.td
64–67

I moved them here as Interfaces use them. I wanted to keep Interfaces and OpBase independent and include Interfaces in OpBase, so we do not break includes for TableGen users.

If we decide to move these to OpBase, Interfaces.td will have to include OpBase, and everyone using interfaces has to fix includes.

Is breaking users something we want to do with this patch? I can add a TODO, and do it in a smaller, separate patch which breaks includes if you prefer that.

Sorry, completely missed there was something for me to action on.

mlir/include/mlir/IR/Utils.td
64–67

ins/outs I can understand, but do interfaces use region or successor? Those are the ones I was talking about.

Groverkss updated this revision to Diff 546375.Aug 2 2023, 2:58 AM
Groverkss marked an inline comment as done.
  • Move successor/region marker to OpBase.td
mlir/include/mlir/IR/Utils.td
64–67

That makes sense. Fixed!

@rriddle I am now waiting for you to accept/request changes on this patch :) (Wanted to make it clear since I did not make it clear last time).

@Groverkss, my understanding is that since River accepted the patch, you should be able to land it as long as you fixed his comments (which I feel you did).

@Groverkss, my understanding is that since River accepted the patch, you should be able to land it as long as you fixed his comments (which I feel you did).

Right. Thanks, I'll land it then.

This revision was automatically updated to reflect the committed changes.