fix: add pto.tfillpad_inplace; stop tfillpad from auto-lowering to TFILLPAD_INPLACE#473
fix: add pto.tfillpad_inplace; stop tfillpad from auto-lowering to TFILLPAD_INPLACE#473FangRui0 wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the TFillPadInplaceOp operation to explicitly handle in-place fill padding, separating this logic from the standard TFillPadOp. The changes include the ODS definition, verifier implementation, and lowering patterns to EmitC. Review feedback identifies a critical syntax error in the new EmitC lowering pattern where a return statement and closing braces are missing. Additionally, improvements are suggested to use the more idiomatic replaceOpWithNewOp in conversion patterns and to move IRRewriter instantiation outside of loops to improve performance.
| LogicalResult matchAndRewrite(pto::TFillPadInplaceOp op, OpAdaptor adaptor, | ||
| ConversionPatternRewriter &rewriter) const override { | ||
| auto loc = op.getLoc(); | ||
|
|
||
| Value src = peelUnrealized(adaptor.getSrc()); | ||
| Value dst = peelUnrealized(adaptor.getDst()); | ||
|
|
||
| rewriter.create<emitc::CallOpaqueOp>( | ||
| loc, TypeRange{}, "TFILLPAD_INPLACE", | ||
| /*args=*/ArrayAttr{}, /*templateArgs=*/ArrayAttr{}, | ||
| /*operands=*/ValueRange{dst, src}); | ||
|
|
There was a problem hiding this comment.
The matchAndRewrite implementation for PTOFillPadInplaceToEmitC is incomplete. It is missing the return statement and the closing braces for both the function and the struct. This will cause a compilation error. Additionally, using replaceOpWithNewOp is preferred.
LogicalResult matchAndRewrite(pto::TFillPadInplaceOp op, OpAdaptor adaptor,
ConversionPatternRewriter &rewriter) const override {
auto loc = op.getLoc();
Value src = peelUnrealized(adaptor.getSrc());
Value dst = peelUnrealized(adaptor.getDst());
rewriter.replaceOpWithNewOp<emitc::CallOpaqueOp>(
op, TypeRange{}, "TFILLPAD_INPLACE",
/*args=*/ArrayAttr{}, /*templateArgs=*/ArrayAttr{},
/*operands=*/ValueRange{dst, src});
return success();
}
};| rewriter.create<emitc::CallOpaqueOp>( | ||
| loc, TypeRange{}, callee, | ||
| loc, TypeRange{}, "TFILLPAD", | ||
| /*args=*/ArrayAttr{}, /*templateArgs=*/ArrayAttr{}, | ||
| /*operands=*/ValueRange{dst, src}); | ||
|
|
||
| rewriter.eraseOp(op); | ||
| return success(); |
There was a problem hiding this comment.
Using replaceOpWithNewOp is more idiomatic in MLIR conversion patterns than manually creating an operation and then erasing the original one. It ensures that the conversion driver is correctly notified of the replacement.
| rewriter.create<emitc::CallOpaqueOp>( | |
| loc, TypeRange{}, callee, | |
| loc, TypeRange{}, "TFILLPAD", | |
| /*args=*/ArrayAttr{}, /*templateArgs=*/ArrayAttr{}, | |
| /*operands=*/ValueRange{dst, src}); | |
| rewriter.eraseOp(op); | |
| return success(); | |
| rewriter.replaceOpWithNewOp<emitc::CallOpaqueOp>( | |
| op, TypeRange{}, "TFILLPAD", | |
| /*args=*/ArrayAttr{}, /*templateArgs=*/ArrayAttr{}, | |
| /*operands=*/ValueRange{dst, src}); | |
| return success(); |
| for (auto op : fillpadInplaceOps) { | ||
| IRRewriter rewriter(ctx); |
There was a problem hiding this comment.
Instantiating an IRRewriter inside a loop is inefficient. It is better to create the rewriter once outside the loop and update its insertion point for each operation.
| for (auto op : fillpadInplaceOps) { | |
| IRRewriter rewriter(ctx); | |
| IRRewriter rewriter(ctx); | |
| for (auto op : fillpadInplaceOps) { |
…ILLPAD_INPLACE Issue hw-native-sys#470: TFILLPAD_INPLACE requires same backing storage with distinct src/dst valid-region semantics; same SSA tfillpad must use TFILLPAD. Add explicit tfillpad_inplace op and lowering to TFILLPAD_INPLACE. Made-with: Cursor
|
/run a3 |
A3 板测失败
日志尾部 |
|
/run a3 |
A3 板测完成（有跳过）
|
2 participants
Issue #470
TFILLPAD_INPLACE requires same backing storage with distinct src/dst valid-region semantics; same SSA tfillpad must use TFILLPAD. Add explicit tfillpad_inplace op and lowering to TFILLPAD_INPLACE.