[P4HIR] Fix unsound CastOp chain folding (#317)#333
[P4HIR] Fix unsound CastOp chain folding (#317)#333AbdallahRashed wants to merge 1 commit intop4lang:mainfrom
Conversation
Cast chains A→B→C were unconditionally folded to A→C, but this changes semantics when B alters signedness before a widening cast (e.g. bit<8>→int<8>→int<16> sign-extends, but bit<8>→int<16> zero-extends). Only fold when the second cast doesn't widen, or the first cast preserves signedness without truncating. Fixes p4lang#317
AbdallahRashed
force-pushed
the
fix_castop_canonicalization
branch
from
f6c8a4c to
29ea2fd
Compare
|
I am opening this PR, to express my interest in GSOC: [Project 3.1: Alkali-P4MLIR: Bridging P4 and SmartNICs Through MLIR Dialect Conversion Between Alkali IR and P4MLIR] |
mtsamis
left a comment
mtsamis
left a comment
There was a problem hiding this comment.
Please see some adjustments to make
| /// 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; |
There was a problem hiding this comment.
Shouldn't be needed due to what we have in CastOp::fold
| return wA <= wB && srcBits.isSigned() == midBits.isSigned(); | ||
| } | ||
|
|
||
| // For non-BitsType chains, be conservative and don't fold. |
There was a problem hiding this comment.
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.
| /// 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. |
There was a problem hiding this comment.
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).
2 participants
Cast chains A→B→C were unconditionally folded to A→C, but this changes semantics when B alters signedness before a widening cast (e.g. bit<8>→int<8>→int<16> sign-extends, but bit<8>→int<16> zero-extends).
Only fold when the second cast doesn't widen, or the first cast preserves signedness without truncating.
Fixes #317