Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement OpAsmInterface for TileElement to get good debug SSA names #718

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Nov 4, 2023

MLIR has an interface that helps with debugging (OpAsmOpInterface) so let's use it on TileElements (for which I've seen many tests that hard code the indices of the tile...). The result (no pun intended) is that

%t21 = AIE.tile(2, 1)
%t20 = AIE.tile(2, 0)
%c21 = AIE.core(%t21)  {
  AIE.end
}
%s21 = AIE.switchbox(%t21)  {
  AIE.connect<Core : 0, South : 0>
}
%s20 = AIE.switchbox(%t20)  {
  AIE.connect<North : 0, South : 2>
}
%mux = AIE.shimmux(%t20)  {
  AIE.connect<North : 2, DMA : 0>
}
%dma = AIE.shimDMA(%t20)  {
  AIE.end
}
AIE.wire(%s21 : South, %s20 : North)
AIE.wire(%s20 : South, %mux : North)
AIE.wire(%mux : DMA, %dma : DMA)
AIE.wire(%mux : South, %t20 : DMA)
AIE.wire(%s21 : Core, %c21 : Core)
AIE.wire(%s21 : Core, %t21 : Core)

pretty prints to

%tile_2_1 = AIE.tile(2, 1)
%tile_2_0 = AIE.tile(2, 0)
%core_2_1 = AIE.core(%tile_2_1) {
  AIE.end
}
%switchbox_2_1 = AIE.switchbox(%tile_2_1) {
  AIE.connect<Core : 0, South : 0>
}
%switchbox_2_0 = AIE.switchbox(%tile_2_0) {
  AIE.connect<North : 0, South : 2>
}
%shimmux_2_0 = AIE.shimmux(%tile_2_0) {
  AIE.connect<North : 2, DMA : 0>
}
%shimDMA_2_0 = AIE.shimDMA(%tile_2_0) {
  AIE.end
}
AIE.wire(%switchbox_2_1 : South, %switchbox_2_0 : North)
AIE.wire(%switchbox_2_0 : South, %shimmux_2_0 : North)
AIE.wire(%shimmux_2_0 : DMA, %shimDMA_2_0 : DMA)
AIE.wire(%shimmux_2_0 : South, %tile_2_0 : DMA)
AIE.wire(%switchbox_2_1 : Core, %core_2_1 : Core)
AIE.wire(%switchbox_2_1 : Core, %tile_2_1 : Core)
AIE.flow(%tile_2_1, Core : 0, %shimDMA_2_0, DMA : 0)

automatically.

The only thing that really needed to be done to make this work is a little fiddling with how interface inheritance works in MLIR1.

Note, the first commit changes a lot of tests that were not using capturing so it couldn't be verified that the change was correct (because the names for the SSA values were %0, %1,..., i.e. hardcoded). By the way, I generated the capturing version of the tests automatically using generate-test-checks.py, e.g.

aie-opt --aie-lower-broadcast-packet test/create-broadcast-packet/test_broad_packet.mlir | generate-test-checks.py --source test/create-broadcast-packet/test_broad_packet.mlir -i --source_delim_regex='module @'

It would be good if we used this from now on.

Footnotes

  1. The TileElement interface has everything needed to implement getAsmResultNames but there's no simple way to share that implementation across ops that subscribe to the interface. So you need to stick the implementation in the trait associated with interface (hence extraTraitClassDeclaration). Then our classes end up looking like this

    class MemOp : public ::mlir::Op<..., ::mlir::OpAsmOpInterface::Trait, ::xilinx::AIE::TileElement::Trait, ...>
    

    which is almost right except with both ::mlir::OpAsmOpInterface::Trait and ::xilinx::AIE::TileElement::Trait having implementations of getAsmResultNames (OpAsmOpInterface::Trait has a default implementation) the call (to getAsmResultNames) is ambiguous. Hence the using declaration.

def AIE_TileOp: AIE_Op<"tile", [FlowEndPoint]>, Results<(outs Index:$result)> {
def AIE_TileOp: AIE_Op<"tile", [
FlowEndPoint,
DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TileOp isn't a TileElement o.O but nonetheless can implement the interface.

@makslevental makslevental changed the title implement OpAsmInterface for TileElement implement OpAsmInterface for TileElement to get good debug SSA names Nov 4, 2023
@makslevental makslevental force-pushed the op_asm_interface branch 2 times, most recently from 7dd9a57 to 9892666 Compare November 7, 2023 08:05
@makslevental makslevental merged commit d21aa51 into main Nov 7, 2023
6 checks passed
@makslevental makslevental deleted the op_asm_interface branch November 7, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant