Page MenuHomePhabricator

[MLIR] Rename SideEffects.td -> SideEffectInterfaces.td
ClosedPublic

Authored by stephenneuendorffer on Wed, May 6, 12:55 PM.

Details

Summary

This normalize the name of the tablegen file with the name of the generated
files (SideEffectInterfaces.h.inc) and the other Interface tablegen files,
which all end in Interface(s).td

Diff Detail

Event Timeline

mehdi_amini added inline comments.Wed, May 6, 1:09 PM
mlir/examples/standalone/include/Standalone/StandaloneOps.td
13

Since "Interface" is already in the path, should we instead normalize the other way?

stephenneuendorffer added a comment.EditedWed, May 6, 2:14 PM

The dialect interfaces don't necessary exist in an "Interfaces" path. (Although perhaps they should?)

./examples/toy/Ch4/include/toy/ShapeInferenceInterface.td
./examples/toy/Ch5/include/toy/ShapeInferenceInterface.td
./examples/toy/Ch6/include/toy/ShapeInferenceInterface.td
./examples/toy/Ch7/include/toy/ShapeInferenceInterface.td
./include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td
./include/mlir/IR/OpAsmInterface.td
./include/mlir/Interfaces/InferTypeOpInterface.td
./include/mlir/Interfaces/DerivedAttributeOpInterface.td
./include/mlir/Interfaces/LoopLikeInterface.td
./include/mlir/Interfaces/ViewLikeInterface.td
./include/mlir/IR/SymbolInterfaces.td
./include/mlir/Interfaces/CallInterfaces.td
./include/mlir/Interfaces/ControlFlowInterfaces.td
./include/mlir/Interfaces/SideEffectInterfaces.td

ftynse accepted this revision.Thu, May 7, 2:02 AM

The dialect interfaces don't necessary exist in an "Interfaces" path. (Although perhaps they should?)

I would differentiate between dialect-specific interfaces (e.g., LinalgStructured) and dialect-independent. The former should live with their dialect similarly to other dialect-specific stuff.

Regarding this change, I am always for consistency and since it makes the code base more consistent, I support the change even if we decide to mass-rename everything later.

herhut accepted this revision.Thu, May 7, 4:51 AM

I agree with @ftynse here.

The dialect interfaces don't necessary exist in an "Interfaces" path. (Although perhaps they should?)

I would differentiate between dialect-specific interfaces (e.g., LinalgStructured) and dialect-independent. The former should live with their dialect similarly to other dialect-specific stuff.

I agree... I think pertaining to the comment from @mehdi_amini , if dialect dependent interfaces always live in Dialect/Foo/Interfaces, then they don't need to include "Interfaces" in the name.

Regarding this change, I am always for consistency and since it makes the code base more consistent, I support the change even if we decide to mass-rename everything later.

Good point here...

@jpienaar Thoughts? You have a blocking review.

jpienaar accepted this revision.Fri, May 8, 9:39 AM

@jpienaar Thoughts? You have a blocking review.

My preference would be same as Mehdi (don't repeat interfaces if already in the name/path). And we could make this consistent by renaming all of the others ones instead (and moving dialect ones). This is smaller changes that makes these consistent and if we then follow up with larger restructuring to make them all consistent and can remove the redundancy in the naming SGTM (we've had this argument before and I know some consider path as part of identifier and some don't - so my view of redundancy is not necessarily uniform).

Given this makes it consistent with the generated tblgen outputs too, this seems like easier start that gets us to consistent state for NFC moves later. So agree with @ftynse and @herhut that this maintains the current naming of the others and makes it more consistent.

This revision is now accepted and ready to land.Fri, May 8, 9:39 AM
This revision was automatically updated to reflect the committed changes.