-
Notifications
You must be signed in to change notification settings - Fork 22
[P4HIR] Fix unsound CastOp chain folding (#317) #333
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -250,12 +250,57 @@ OpFoldResult P4HIR::CastOp::fold(FoldAdaptor) { | |
| return {}; | ||
| } | ||
|
|
||
| /// Returns true if folding a cast chain A -> B -> C into A -> C preserves | ||
| /// the cast semantics | ||
| /// | ||
| /// The key unsafe case is when the second cast widens (w_B < w_C), because | ||
| /// the extension type (zero vs sign) depends on the source's signedness. | ||
| /// If the intermediate type B differs from A in a way that changes how | ||
| /// the widening is performed, the fold would alter semantics. | ||
| /// | ||
| /// Fold is safe when: | ||
| /// - w_B >= w_C: second cast doesn't widen (truncation and reinterpretation | ||
| /// are sign-independent), OR | ||
| /// - w_A <= w_B AND s_A == s_B: first cast doesn't truncate and preserves | ||
| /// signedness, so the widening in the second cast uses the same extension | ||
| /// type as the direct A -> C cast would. | ||
| static bool isSafeCastComposition(mlir::Type srcType, mlir::Type midType, mlir::Type dstType) { | ||
| if (srcType == midType || midType == dstType) return true; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't be needed due to what we have in CastOp::fold |
||
|
|
||
| auto srcBits = mlir::dyn_cast<P4HIR::BitsType>(srcType); | ||
| auto midBits = mlir::dyn_cast<P4HIR::BitsType>(midType); | ||
| auto dstBits = mlir::dyn_cast<P4HIR::BitsType>(dstType); | ||
|
|
||
| if (srcBits && midBits && dstBits) { | ||
| unsigned wA = srcBits.getWidth(); | ||
| unsigned wB = midBits.getWidth(); | ||
| unsigned wC = dstBits.getWidth(); | ||
|
|
||
| // Safe if the second cast doesn't widen. | ||
| if (wB >= wC) return true; | ||
|
|
||
| // Second cast widens (wB < wC). Safe only if the first cast doesn't | ||
| // truncate and preserves signedness, so the composed extension matches | ||
| // the direct A -> C extension. | ||
| return wA <= wB && srcBits.isSigned() == midBits.isSigned(); | ||
| } | ||
|
|
||
| // For non-BitsType chains, be conservative and don't fold. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please make this more complete by also handling booleans? They should be fairly easy to add and then you can also remove this comment. |
||
| return false; | ||
| } | ||
|
|
||
| LogicalResult P4HIR::CastOp::canonicalize(P4HIR::CastOp op, PatternRewriter &rewriter) { | ||
| // Composition. | ||
| // %b = cast(%a) : A -> B | ||
| // cast(%b) : B -> C | ||
| // ===> cast(%a) : A -> C | ||
| if (auto inputCast = mlir::dyn_cast_if_present<CastOp>(op.getSrc().getDefiningOp())) { | ||
| mlir::Type srcType = inputCast.getSrc().getType(); | ||
| mlir::Type midType = inputCast.getType(); | ||
| mlir::Type dstType = op.getType(); | ||
|
|
||
| if (!isSafeCastComposition(srcType, midType, dstType)) return failure(); | ||
|
|
||
| auto bitcast = | ||
| rewriter.createOrFold<P4HIR::CastOp>(op.getLoc(), op.getType(), inputCast.getSrc()); | ||
| rewriter.replaceOp(op, bitcast); | ||
|
|
||
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 think this middle section of comments is overly verbose.
The safe cases are explained below and naming this the "key unsafe case" is somewhat misleading (I would say truncation is also key).