-
Notifications
You must be signed in to change notification settings - Fork 444
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
Strength reduction should not remove some casts #3226
Conversation
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Fixed #3225 |
frontends/p4/strengthReduction.cpp
Outdated
if (expr->getH() < size_t(cast->expr->type->width_bits())) { | ||
expr->e0 = cast->expr; | ||
if (auto tb = cast->expr->type->to<IR::Type_Bits>()) { | ||
auto type = expr->type->to<IR::Type_Bits>(); |
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.
Why not use checkedTo here?
frontends/p4/strengthReduction.cpp
Outdated
if (auto tb = cast->expr->type->to<IR::Type_Bits>()) { | ||
auto type = expr->type->to<IR::Type_Bits>(); | ||
CHECK_NULL(type); | ||
if (tb->isSigned != type->isSigned) |
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 always assumed that slices of signed bit strings will produce an unsigned bit string. This should probably ensure that a cast to int<W>
is converted into a bit<W>
. A cast from int<W>
to bit<W>
should be acceptable though?
This is what the spec says for slicing int<W>
:
The result is an unsigned bit-string of width m - l + 1, including the bits numbered from l (which becomes the least significant bit of the result) to m (the most significant bit of the result) from the source operand.
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 added another test case which fails without this change; it's in the test file:
h.h.b = ((bit<1>)h.h.c)[0:0];
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.
But
((bit<1>)h.h.c)[0:0];
to
((bit<1>)h.h.c);
should be acceptable, right?
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 previous result was just h.h.c, which failed to typecheck.
The only thing is to to add a new member d to the header and assign c to that one. Otherwise, the assignment is optimized away. |
Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
Is this ready to be merged? |
Signed-off-by: Mihai Budiu mbudiu@vmware.com