-
Notifications
You must be signed in to change notification settings - Fork 86
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
Fixing up PLIO compiler support and creating an example #1623
Conversation
Coverage ReportCreated: 2024-07-23 02:13Click here for information about interpreting this report.
Generated by llvm-cov -- llvm version 14.0.0 |
@@ -1634,7 +1636,8 @@ def AIE_ObjectFifoCreateOp: AIE_Op<"objectfifo", [HasParent<"DeviceOp">, Symbol] | |||
TypeAttrOf<AIE_ObjectFifoType>:$elemType, | |||
BDDimLayoutArrayAttr:$dimensionsToStream, | |||
BDDimLayoutArrayArrayAttr:$dimensionsFromStreamPerConsumer, | |||
DefaultValuedAttr<BoolAttr, "false">:$via_DMA | |||
DefaultValuedAttr<BoolAttr, "false">:$via_DMA, | |||
DefaultValuedAttr<BoolAttr, "false">:$plio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit odd to me.. Can't the objectFifo figure out that it's connected to PLIO based on the shimDMAAllocation op? Could I have an objectFifo connecting 2 PLIOs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using this as a way for the user to specify whether the corresponding shim tile(s) of an object FIFO should be connected to GMIO or PLIO. Here is an example: https://github.com/Xilinx/mlir-aie/blob/plio-rebase/programming_examples/basic/passthrough_dmas_plio/aie2-input-plio.py#L49.
We do also need to encode this in the shimDMAAllocationOP but that is later in the compilation, and I believe usually not exposed via the IRON python bindings.
I believe an object FIFO could connect 2 PLIOs but have never tested it. It can connect two GMIOs, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shimDMAAllocationOp
is currently being generated by the AIEObjectFifoStatefulTransform and as the object FIFO also abstracts the AIEFlowOps
between tiles I think it can be confusing to know where to add new attributes to guide the configuration. A cleaner solution might be to "allow" shimDMAAllocationOps
to exist before the object FIFO lowering pass and have the lowering pass check for them and either: adopt whatever configuration they specify for the shim tile (including the channel index), or completing any missing information if it was not provided (following the same generation it currently uses). Another possibility would be to rely on the current AIEFlowOp
in the same manner as above for a more general solution that would allow users to specify the ports they want for any tile not just the shim tile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either works for me the only important thing to me is that this is user facing. That being said, that change in allowing shimDMAAllocationOps
to be before the object FIFO lowering pass sounds like it could require some refactoring. If it is non-trivial, should that be a separate PR? If so, I can create an issue so we can track it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go this route, it will indeed require a bit of refactoring and I do think it will be better to have it in a separate PR that this PR would then build on. Let's discuss further and see if any of those solutions would be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cleaner solution might be to "allow" shimDMAAllocationOps to exist before the object FIFO lowering pass
+1 because it makes things more composable
lib/Targets/AIETargetXAIEV2.cpp
Outdated
if (connectOp.getDestBundle() == WireBundle::North) | ||
// mux | ||
output << "__mlir_aie_try(XAie_PlToAieIntfEnable(" << deviceInstRef | ||
<< ", " << tileLocStr("x", "y") << ", " | ||
<< connectOp.destIndex() << ", PLIF_WIDTH_64));\n"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the case of being connected TO a PLIO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we are only supporting the 0th and 1st channel as mentioned in
mlir-aie/lib/Targets/AIETargetXAIEV2.cpp
Line 757 in e05bff4
"Currently only PLIO channel 0 and 1 are supported."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, would like to see the proper configuration happen here, since there's no fundamental reason why the other cases can't be implemented as well. At the very least, this should be an error rather than a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, it was originally an error but that made one of the plio unit tests fail. The reason that I didn't put them in there is that it is difficult to test as I am not sure how at platform build time to force a PLIO connection to use a specific channel. I will look into it and if that doesn't work, I will at least make it an error and change the unit test that was failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0f50197 Removed the channel limitation in this commit.
rewriter, cast<Interconnect>(shimMuxOp.getOperation()), | ||
flowOp, srcBundle, srcChannel, WireBundle::North, shimCh); | ||
} | ||
ShimMuxOp shimMuxOp = analyzer.getShimMux(rewriter, srcSB.col); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me what the point of this change is, but I don't understand the original code necessarily. Is this related to the fact that you only 'support' channel 0 and 1 in the code below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how correct the previous code was, but in the pass here:
mlir-aie/lib/Targets/AIETargetXAIEV2.cpp
Lines 709 to 767 in d08090b
for (auto op : targetOp.getOps<ShimMuxOp>()) { | |
Region &r = op.getConnections(); | |
Block &b = r.front(); | |
bool isEmpty = b.getOps<ConnectOp>().empty(); | |
if (isa<TileOp>(op.getTile().getDefiningOp())) { | |
int col = op.colIndex(); | |
int row = op.rowIndex(); | |
if (!isEmpty) { | |
output << "// ShimMux column " << col << " row " << row << "\n"; | |
output << "// NOTE ShimMux always connects from the south as " | |
<< "directions are defined relative to the tile stream " | |
<< "switch\n"; | |
output << "x = " << col << ";\n"; | |
output << "y = " << row << ";\n"; | |
} | |
} | |
for (auto connectOp : b.getOps<ConnectOp>()) { | |
if(connectOp.getSourceBundle() == WireBundle::DMA || connectOp.getDestBundle() == WireBundle::DMA) { | |
if (connectOp.getSourceBundle() == WireBundle::North) | |
// demux! | |
output | |
<< "__mlir_aie_try(XAie_EnableAieToShimDmaStrmPort(" | |
<< deviceInstRef << ", " << tileLocStr("x", "y") | |
<< ", " | |
// << | |
// stringifyWireBundle(connectOp.sourceBundle()).upper() | |
<< connectOp.sourceIndex() << "));\n"; | |
else if (connectOp.getDestBundle() == WireBundle::North) | |
// mux | |
output | |
<< "__mlir_aie_try(XAie_EnableShimDmaToAieStrmPort(" | |
<< deviceInstRef << ", " << tileLocStr("x", "y") | |
<< ", " | |
// << | |
// stringifyWireBundle(connectOp.sourceBundle()).upper() | |
<< connectOp.destIndex() << "));\n"; | |
} | |
else if(connectOp.getSourceBundle() == WireBundle::PLIO || connectOp.getDestBundle() == WireBundle::PLIO) { | |
// Note: Right now this just works with PLIO channel 0 and 1 as those don't require to program | |
// the shim mux | |
if(connectOp.destIndex() != 0 && connectOp.destIndex() != 1) { | |
return connectOp.emitOpError("Currently only PLIO channel 0 and 1 are supported."); | |
} | |
if (connectOp.getDestBundle() == WireBundle::North) | |
// mux | |
output | |
<< "__mlir_aie_try(XAie_PlToAieIntfEnable(" | |
<< deviceInstRef << ", " << tileLocStr("x", "y") | |
<< ", " | |
<< connectOp.destIndex() | |
<< ", PLIF_WIDTH_64));\n"; | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I get it. I think the original code here was conceptually incorrect. The intention is/was to be explicit about all the connections that exist even the non-programmable ones. Conceptually the 'shimMux' op is intended to represent all of the connectivity in the shim south of the stream switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pulling this together!
rewriter, cast<Interconnect>(shimMuxOp.getOperation()), | ||
flowOp, srcBundle, srcChannel, WireBundle::North, shimCh); | ||
} | ||
ShimMuxOp shimMuxOp = analyzer.getShimMux(rewriter, srcSB.col); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I get it. I think the original code here was conceptually incorrect. The intention is/was to be explicit about all the connections that exist even the non-programmable ones. Conceptually the 'shimMux' op is intended to represent all of the connectivity in the shim south of the stream switch.
lib/Targets/AIETargetXAIEV2.cpp
Outdated
if (connectOp.getDestBundle() == WireBundle::North) | ||
// mux | ||
output << "__mlir_aie_try(XAie_PlToAieIntfEnable(" << deviceInstRef | ||
<< ", " << tileLocStr("x", "y") << ", " | ||
<< connectOp.destIndex() << ", PLIF_WIDTH_64));\n"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, would like to see the proper configuration happen here, since there's no fundamental reason why the other cases can't be implemented as well. At the very least, this should be an error rather than a warning.
@@ -1634,7 +1636,8 @@ def AIE_ObjectFifoCreateOp: AIE_Op<"objectfifo", [HasParent<"DeviceOp">, Symbol] | |||
TypeAttrOf<AIE_ObjectFifoType>:$elemType, | |||
BDDimLayoutArrayAttr:$dimensionsToStream, | |||
BDDimLayoutArrayArrayAttr:$dimensionsFromStreamPerConsumer, | |||
DefaultValuedAttr<BoolAttr, "false">:$via_DMA | |||
DefaultValuedAttr<BoolAttr, "false">:$via_DMA, | |||
DefaultValuedAttr<BoolAttr, "false">:$plio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shimDMAAllocationOp
is currently being generated by the AIEObjectFifoStatefulTransform and as the object FIFO also abstracts the AIEFlowOps
between tiles I think it can be confusing to know where to add new attributes to guide the configuration. A cleaner solution might be to "allow" shimDMAAllocationOps
to exist before the object FIFO lowering pass and have the lowering pass check for them and either: adopt whatever configuration they specify for the shim tile (including the channel index), or completing any missing information if it was not provided (following the same generation it currently uses). Another possibility would be to rely on the current AIEFlowOp
in the same manner as above for a more general solution that would allow users to specify the ports they want for any tile not just the shim tile.
…check in plio_shimmux test
… a tile where there is no shim dma
73d9591
to
16dd328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with the intent to refactor some of the code once the PR (to be linked..) generalizing the use of shimDMAAllocationOps
as discussed is merged.
Thank you for the changes Eddie!
* update llvm * update for Xilinx/mlir-aie#1623 * update for aiex.runtime_sequence * bump llvm, mlir-aie * bump mlir-aie
Adding the necessary passes to have a
plio
attribute in an aie2.py and have that lower to connecting the PLIO on the VCK5000 platform.This will be coupled with a PR in the platforms repo with a platform that has PLIO connected on the VCK5000.