Add more checks for multiplication results #16146
Conversation
|
Hi @knightXun |
|
Hi @knightXun |
8697315 to
e145f3b
Compare
|
@NaiyerRizz nicely ping. |
|
Hi @penpornk |
|
@beckerhe Can you help me review this PR? |
|
Hi, @reedwm can you take a look or forward this to someone? |
| case HloOpcode::kExp: | ||
| return true; |
There was a problem hiding this comment.
Exp can be zero if the argument is negative infinity. Similarly, for the added kMultiply case where the LHS and RHS are positive, the result can underflow to zero.
Hmmm, but I see existing cases have similar problems, even without this PR. E.g. kPower can also be zero if the RHS is negative infinity. Perhaps we don't care about cases where underflow happen, or where arguments are infinity? @akuegel, do you have an opinion here?
There was a problem hiding this comment.
Sorry, I somehow missed this question. Good point, I think we should not return true for kExp. And same for kPower. We can return true for them in the IsNonNegative function.
Also, good catch regarding underflow to zero, I think we should also not return true for kMultiply.
| if (!IsNonNegative(hlo->operand(0), options) && | ||
| !IsNonNegative(hlo->operand(1), options)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Remove this case. If either the LHS or RHS is zero, the output will be zero, not positive.
|
Hi @knightXun |
There was a problem hiding this comment.
@knightXun please move these checks to the IsNonNegative function. Like @reedwm pointed out, we can have zero results.
| case HloOpcode::kExp: | ||
| return true; |
There was a problem hiding this comment.
Sorry, I somehow missed this question. Good point, I think we should not return true for kExp. And same for kPower. We can return true for them in the IsNonNegative function.
Also, good catch regarding underflow to zero, I think we should also not return true for kMultiply.
|
@knightXun I see this PR has been inactive for a while. If you're still interested in merging it, could you please do the requested changes above? Otherwise, we will close it after some more time of inactivity. |
Add positive checks for multiplication results
This commit introduces a new function that determines if the result of a multiplication is a positive number. It handles the cases where both operands are negative or both are positive, ensuring that the result is treated as a positive number if the product of two negatives is involved.
The function checks the sign of the operands and applies the mathematical rule that the product of two negative numbers is positive. This is crucial for ensuring the correctness of calculations in various scenarios, such as financial calculations or scientific computations where the sign of the result is significant.