Skip to content
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.

Sign up for GitHub

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

Fix how overflow property is set for arithmetic operations during ValuePropagation #7574

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix how overflow property is set for arithmetic operations during ValuePropagation #7574

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0396cc
Fix int arithmetic op nodes being incorrectly marked as cannotOverflow
nbhuiyan Nov 29, 2024
0b60014
Fix long arithmetic op nodes being incorrectly marked as cannotOverflow
nbhuiyan Nov 29, 2024
d6a7f66
Fix short arithmetic op nodes being incorrectly marked as cannotOverflow
nbhuiyan Nov 29, 2024
7102873
Clean up commented-out code in checkForNonNegativeAndOverflowProperties
nbhuiyan Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions compiler/optimizer/VPHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ static void checkForNonNegativeAndOverflowProperties(OMR::ValuePropagation *vp,
if (node->getOpCode().isLoad())
{
node->setCannotOverflow(true);
//dumpOptDetails(vp->comp(), "Load node %p cannot overflow\n", node);
}

if (constraint)
Expand Down Expand Up @@ -144,15 +143,14 @@ static void checkForNonNegativeAndOverflowProperties(OMR::ValuePropagation *vp,
if (high <= 0)
node->setIsNonPositive(true);

///if ((node->getOpCode().isArithmetic() || node->getOpCode().isLoad()) &&
/// ((low > TR::getMinSigned<TR::Int32>()) || (high < TR::getMaxSigned<TR::Int32>())))
if ((node->getOpCode().isLoad() &&
((low > TR::getMinSigned<TR::Int32>()) || (high < TR::getMaxSigned<TR::Int32>()))) ||
(node->getOpCode().isArithmetic() &&
(range->canOverflow() != TR_yes)))
if (node->getOpCode().isLoad() &&
((low > TR::getMinSigned<TR::Int32>()) || (high < TR::getMaxSigned<TR::Int32>())))
{
node->setCannotOverflow(true);
//dumpOptDetails(vp->comp(), "Node %p cannot overflow\n", node);
}
else if (node->getOpCode().isArithmetic())
{
node->setCannotOverflow(false);
Comment on lines +151 to +153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this change will definitely avoid the problem, I think it's going to be too heavy-handed. Consider this example:

public class Experiment {
    public static final int sub(int i, int j) {
        if (i > 0 && i < 100 && j > 0 && j < 100) {
            return i*j + i/j;
        }
        return 0;
    }

    public static final void main(String[] args) {
        sub(1, 2);
        sub(1, 2);
    }
}

Global Value Propagation is able to determine the ranges of the arithmetic operations, and it ought to be able to determine from that that they cannot overflow.

   n32n idiv gets new constraint: value 5 is (0 to 99)I
   n29n imul gets new constraint: value 6 is (1 to 9801)I
   n34n iadd gets new constraint: value 7 is (1 to 9900)I

The existing code here is testing whether range->canOverflow() != TR_yes. I'm wondering whether the constraint that's associated with the imul can have its range->canOverflow() return TR_yes. I think the fix will likely be a bit more involved to make that happen.

}
}
else if (constraint->asLongRange())
Expand All @@ -166,15 +164,14 @@ static void checkForNonNegativeAndOverflowProperties(OMR::ValuePropagation *vp,
if (high <= 0)
node->setIsNonPositive(true);

///if ((node->getOpCode().isArithmetic() || node->getOpCode().isLoad()) &&
/// ((low > TR::getMinSigned<TR::Int64>()) || (high < TR::getMaxSigned<TR::Int64>())))
if ((node->getOpCode().isLoad() &&
((low > TR::getMinSigned<TR::Int64>()) || (high < TR::getMaxSigned<TR::Int64>()))) ||
(node->getOpCode().isArithmetic() &&
(range->canOverflow() != TR_yes)))
if (node->getOpCode().isLoad() &&
((low > TR::getMinSigned<TR::Int64>()) || (high < TR::getMaxSigned<TR::Int64>())))
{
node->setCannotOverflow(true);
//dumpOptDetails(vp->comp(), "Node %p cannot overflow\n", node);
}
else if(node->getOpCode().isArithmetic())
{
node->setCannotOverflow(false);
}
}
else if (constraint->asShortRange())
Expand All @@ -189,11 +186,14 @@ static void checkForNonNegativeAndOverflowProperties(OMR::ValuePropagation *vp,
if (high <= 0)
node->setIsNonPositive(true);

if ((node->getOpCode().isLoad() && ((low > TR::getMinSigned<TR::Int16>()) || (high < TR::getMaxSigned<TR::Int16>()))) ||
(node->getOpCode().isArithmetic() && (range->canOverflow() != TR_yes)))
if ((node->getOpCode().isLoad() && ((low > TR::getMinSigned<TR::Int16>()) || (high < TR::getMaxSigned<TR::Int16>()))))
{
node->setCannotOverflow(true);
}
else if (node->getOpCode().isArithmetic())
{
node->setCannotOverflow(false);
}

}
}
Expand Down