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

[Matthias Gehre]

mgehre-amd wrote:

> So in order to support `gpu.launch_func`, `gpu.module` is a symbol. We could add this later, but we might want to plan ahead. >For instance, if we allow any string as the proposed `id` it might later be unusable as a valid symbol name. We could then add a second attribute for the symbol name or pose restrictions on the `id` attribute, with both options breaking backward compatibility of existing MLIR files.

Adding an additional attribute later would not break compatibility. And we don't know yet if we actually going to get this, right?

> 
> Another aspect is the necessity of a dedicated `emitc` op. If its sole purpose is to provide a named scope, would the builtin module suffice (as IINM in the LLVM dialect)?

I think that would work. It would limit us if we want to extend it in the future, e.g. by adding language standards to the scope.
 

> > > (c) Validation: Utilizing standard op validation to verify that all its internal ops are `emitc` and potentially that these ops and types are valid in its designated C-dialect (e.g. vector types, address spaces, templates).
> > 
> > 
> > Adding a validation pass is something I started some time ago but didn't had time to finish. So yes, +1 from me. However, does it influence changed introduced by this patch?
> 
> 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).

> 
> > > Would be good to discuss this op's designated uses to make sure we don't introduce backward compatibility later. WDYT?
> > 
> > 
> > Makes sense, yes.
> 
> 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.

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