[Mlir-commits] [mlir] [MLIR] emitc: Add emitc translation unit op (PR #123298)

[Gil Rapaport]

aniragil wrote:

> > Adding an additional attribute later would not break compatibility. And we don't know yet if we actually going to get this, right?
> 
> Yes, that sounds reasonable to me.
> 
> > > So I was thinking whether the op should have a ::verify() method. Unless the translator itself runs such a verifying pass (which I'm not sure translators are supposed to do) this pass would be optional, letting users call the translator on invalid files. If we implement ::verify() the op would have to be introduced after all lowering to `emitc` has been done. If we do this later on, we might be breaking downstream compilers which expected it to be usable earlier.
> > 
> > 
> > I'm opposed to recursive verification in `::verify` as this is very costly and `::verify` gets called a lot. Others have done this heavy validation via separate validation passes (e.g. TOSA).
> 
> Yes, with `Adding a validation pass` in my comment above I was referring to a validation pass like the one TOSA has to check for compliance with a TOSA spec. A downstream user would still need to run this pass (or add it to a pipeline) but that should be acceptable.
> 
> > > Any other potential uses for this op?
> > 
> > 
> > I think we should be pragmatic here. A source file is a source file. If we need different more elaborate constructs, there is always the option to add new ops, or change existing onces. MLIR/emitc have not been backwards compatible, so we shouldn't make it too hard for us.
> 
> 👍
> 
> > In summary, I see consensus to rename this to `emitc.file`.
> > Is there any objection to afterwards get this merged? I heard many ideas about possible extensions, but I don't see how the current design (an operation with an identifier) would make any of them impossible.

Yes, IIUC we're all OK with changing its behavior later as needed, so let's move forward.

> 
> No objections. I would say, lets go ahead with `emitc.file`.

Agreed.

https://github.com/llvm/llvm-project/pull/123298