check the attribute of new inserted op, ignore if not exists #3
Conversation
update new features implemented in StandardToHandshakeFPGA18
There was a problem hiding this comment.
Thanks for the work! We need to make sure the pass works even when the bb attribute is not defined though (see comments). I know it will introduce some redundant/annoying code but at this point it is the easiest way to handle things. Hopefully in the near future I'll have time to rework how we handle bb attributes so it will be nicer to work with.
lib/Transforms/InitCstWidth.cpp
Outdated
| @@ -67,6 +67,7 @@ static LogicalResult initCstOpBitsWidth(handshake::FuncOp funcOp, | |||
| op.getResult().getType().getIntOrFloatBitWidth() - cstBitWidth; | |||
| auto extOp = builder.create<mlir::arith::ExtSIOp>( | |||
| newCstOp.getLoc(), op.getResult().getType(), newCstOp.getResult()); | |||
| extOp->setAttr("bb", op->getAttr("bb")); | |||
There was a problem hiding this comment.
You need to actually check whether the bb attribute exists on op, otherwise this will fail (it should still be possible to run the pass when the bb attribute us not set). Feel free to define something like this and #include "dynamatic/Conversion/StandardToHandshakeFPGA18.h" to get BB_ATTR in scope.
Then, in your buffer pass, before doing anything you can check that every operation has a bb attribute since (I believe) it's necessary for the pass to run. If at least one operation doesn't have a bb attribute the pass/function should fail gracefully (by calling signalPassFailure()/returning a failed LogicalResult or whatever makes sense in the context).
lib/Transforms/UtilsBitsUpdate.cpp
Outdated
| @@ -47,7 +47,7 @@ std::optional<Operation *> insertWidthMatchOp(Operation *newOp, int opInd, | |||
| auto truncOp = builder.create<mlir::arith::TruncIOp>(newOp->getLoc(), | |||
| newType, opVal); | |||
| newOp->setOperand(opInd, truncOp.getResult()); | |||
|
|
|||
| truncOp->setAttr("bb", newOp->getAttr("bb")); | |||
lib/Transforms/UtilsBitsUpdate.cpp
Outdated
| @@ -57,7 +57,7 @@ std::optional<Operation *> insertWidthMatchOp(Operation *newOp, int opInd, | |||
| auto extOp = | |||
| builder.create<mlir::arith::ExtSIOp>(newOp->getLoc(), newType, opVal); | |||
| newOp->setOperand(opInd, extOp.getResult()); | |||
|
|
|||
| extOp->setAttr("bb", newOp->getAttr("bb")); | |||
There was a problem hiding this comment.
These passes should not fail when bb is not present. Before I meant that the buffer pass should fail in case bb is not defined for an operation, since otherwise it cannot do its job (in opposition to bitwidth optimization, which does not care for its own processing).
lib/Transforms/InitCstWidth.cpp
Outdated
| auto a = op->getAttrs(); | ||
|
|
||
| if (failed(containsAttr(op, BB_ATTR))) | ||
| return failure(); |
There was a problem hiding this comment.
We don't want any of these bitwidth optimization related passes to fail when there is no bb attribute since they do not care about it for their own behavior. They just have to make their best effort to carry it around if it's there, otherwise they can't do anything and will just not give a bb attribute to the newly created operation. The same applies everywhere else in the PR.
Hi Lucas, I just raise an easy but important PR.
We should set bb attributes for inserted extension & truncation operations. As in buffer placement, we rely on the bb attributes to determine whether the operation units is in the subnetlist (selected bb). Otherwise, we cannot determine whether the new inserted op should selected or not, and it would raise errors when using the optimized handshake level file.