[mlir][dxsa] Add AsmPrinter to translate from MLIR to DXSA text#130
[mlir][dxsa] Add AsmPrinter to translate from MLIR to DXSA text#130asavonic wants to merge 6 commits into
Conversation
AsmPrinter traverses all instructions in an module and prints them in "canonical" assembly syntax. There is no description of its grammar, so the patch follows examples from "Direct3D 11.3 Functional Specification" and output and tests from existing DirectX compiler tools, such as DXC. Only standard instructions and some basic operand types are supported for now.
|
|
||
| LogicalResult emitModule(ModuleOp source) { | ||
| Region ®ion = source.getRegion(); | ||
| if (!region.hasOneBlock()) { |
There was a problem hiding this comment.
Do we really need this check here? It seems to be something for verifier to enforce.
There was a problem hiding this comment.
Yes. Can you suggest how to add a verifier? ModuleOp is a builtin operand. Do we need to make a dialect-specific operand?
There was a problem hiding this comment.
Well, then this is even more redundant :)
ModuleOp is single-region single-bb, this is already enforced: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/BuiltinOps.td#L58
Use source.getBody() to get that BB.
Overall, think about IR invariants. Enforcing them in asmprinter is a bad idea. Overall, you should never have emitError here unless you have not supported something yet.
There was a problem hiding this comment.
Oh, that is much better, thank you!
Now we only use emitError for unimplemented features or as a Default case in TypeSwitch for unsupported (new?) operations.
| assert(index && "undefined index"); | ||
|
|
||
| // Non-immediate indices always use subscript syntax. | ||
| if (!isa<dxsa::IndexImm>(*index)) { |
There was a problem hiding this comment.
Maybe remove extra { per code style guide to reduce number of lines?
| } | ||
|
|
||
| StringRef separator = ""; | ||
| for (APInt v : attr) { |
There was a problem hiding this comment.
no need to copy apint here, grab references.
There was a problem hiding this comment.
Missed that, thank you!
| auto attr = cast<DenseIntElementsAttr>(op.getImm()); | ||
| auto elementType = cast<IntegerType>(attr.getType().getElementType()); | ||
|
|
||
| if (elementType.getWidth() != 32) |
There was a problem hiding this comment.
Again, this is something verifier should enforce. This is an IR invariant. You should not check it here.
There was a problem hiding this comment.
Sorry, that should be a FIXME. I don't know how to print 64-bit immediate operands yet, but they are supported in Shader Assembly.
| outs << "l("; | ||
|
|
||
| StringRef separator = ""; | ||
| for (const APInt &v : attr) { |
There was a problem hiding this comment.
Use interleaveComma here: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/STLExtras.h#L2312
| .Case<dxsa::IndexImm>( | ||
| [this](auto index) { return emitIndexImm(index); }) | ||
| .Case<dxsa::IndexRel>( | ||
| [this](auto index) -> LogicalResult { return emitIndexRel(index); }) |
There was a problem hiding this comment.
Why do you need trailing result types here, but not in the first case? Do you need them at all?
There was a problem hiding this comment.
Thanks! These are leftovers from an early iteration where return type was different. Removed.
2 participants
AsmPrintertraverses all instructions in an module and prints them in "canonical" assembly syntax. There is no description of its grammar, so the patch follows examples from "Direct3D 11.3 Functional Specification" and output and tests from existing DirectX compiler tools, such as DXC.Only standard instructions and some basic operand types are supported for now.