This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][SPIRV] Added optional name to SPIR-V module
ClosedPublic

Authored by georgemitenkov on Aug 21 2020, 4:11 PM.

Details

Summary

This patch adds an optional name to SPIR-V module.
This will help with lowering from GPU dialect (so that we
can pass the kernel module name) and will be more naturally
aligned with ModuleOp.

Diff Detail

Event Timeline

georgemitenkov created this revision.Aug 21 2020, 4:11 PM
georgemitenkov requested review of this revision.Aug 21 2020, 4:11 PM

I am not totally opposed to it though. Its one way of making the name optional, but still using the symbol table traits.

@rriddle any thoughts here?

mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
422

It might be better to just add a SymbolTable trait to this (similar to this). THen the spv.module will also be uniqued which is probably more robust.

That would make it a bigger change though. Is this something that is needed for the SPIRVToLLVM conversion? For now there it is not handling the case of multiple spv.module translation anyway, so we can just skip this for now if this takes long. I would assume you have to update a LOT of tests....

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
2288

Nit: Better to add { .. } since the body spans multiple lines.

rriddle added inline comments.Aug 24 2020, 10:32 PM
mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
364

You should add the symbol interface, as it includes support for optional symbols, and provide a bool isOptionalSymbol() { return true; } method:
https://github.com/llvm/llvm-project/blob/05777ab941063192b9ccb1775358a83a2700ccc1/mlir/include/mlir/IR/SymbolInterfaces.td#L118

413

This needs to be sym_name.

439

This can just be Optional<StringRef> getName() { return sym_name(); }.

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
2289

nit: You can do state.attributes.append(mlir::SymbolTable::getSymbolAttrName(), ...)

2305

Same as above.

2316

nit: Drop the mlir::.

georgemitenkov added inline comments.Aug 25 2020, 2:27 AM
mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
422

It makes sense to have spv.module be uniqued by name. I am fine with both ways and I think that updating tests should not be too long :)

This is needed for passing the GPU module name to the SPIR-V module (and later is used in mlir-spirv-cpu-runner to find the necessary SPIR-V module simply by looking it up in a symbol table). This is not 100% needed for the runner, but is a much nicer way to implement SPIR-V module lookup.

mravishankar requested changes to this revision.Aug 25 2020, 2:28 PM

Making this request changes. I think after addressing comments this is ready to land.

This revision now requires changes to proceed.Aug 25 2020, 2:28 PM
georgemitenkov marked 8 inline comments as done.

Addressed comments.

mravishankar accepted this revision.Aug 25 2020, 9:37 PM

Thanks George! Please fix the clang-tidy errors before submitting if possible.

This revision is now accepted and ready to land.Aug 25 2020, 9:37 PM
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
2296

@mravishankar In this case is it fine to leave like that? That's how it was previously so I suppose the snake case is fine here?

Fixed clang-tidy error.

georgemitenkov added inline comments.Aug 26 2020, 8:46 PM
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
2296

I decided to change the names to camel case instead.

This revision was automatically updated to reflect the committed changes.