In general, the following two points are potentially risky in DA:
- Computing the absolute value of a SCEV expression. Calling
`ScalarEvolution::getAbsExpr` on a signed minimum value will overflow
and return the same value, which is not the expected result in DA.
- Performing arithmetic operations involving a backedge-taken count. The
backedge-taken count is meaningful in an unsigned sense, whereas DA
currently treats all other values and operations in a signed sense.
For these reasons, I think it is better to avoid such operations in DA
whenever possible.
This patch addresses these issues in the Weak Zero SIV tests. I
attempted to craft a test case that exposes a defect caused by these
factors, but I couldn't find one. I'm not entirely sure whether the
current implementation is sound, but anyway, I think avoiding such
operations in DA is a good practice to prevent potential issues in the
future.
The trivial disjoint checks in several dependence tests were
consolidated into a single invocation in #187435. However, the
duplicated check still remains in the Weak Crossing SIV tests. This is
redundant and should be deleted as well.
This patch fixes issues where the Exact SIV/RDIV tests can return
incorrect results when the BTC is negative in a signed sense. The root
cause is that BTCs should be interpreted in an unsigned sense, but
`inferAffineDomain` uses the BTC in inequalities that are interpreted in
a signed sense. These inequalities make sense only when the BTC is
non-negative. Thus we need to check it beforehand.
A couple of these variables are only used within LLVM_DEBUG statements
which get removed by the preprocessor in non-assertions builds which
will cause the variable to become unused. Mark them maybe_unused given
the names make the code more readable.
There have existed two functions `weakZeroSrcSIVtest` and
`weakZeroDstSIVtest`, which are almost identical, except for some minor
differences (e.g., the Direction to be updated). This patch consolidates
the shared core logic into a single function `weakZeroSIVtestImpl` then
calls it from both `weakZeroSrcSIVtest` and `weakZeroDstSIVtest`,
passing the appropriate parameters to handle the differences. This
reduces code duplication and improves maintainability.
This patch simplifies the logic used to determine if the `Distance`
is divisible by 2. Previously, this was done by allocating an APInt
and performing a signed remainder (`srem`) operation.
Since `Distance` is an APInt, we can more efficiently check if it
is odd by directly inspecting the least significant bit (`Distance[0]`).
This avoids an expensive division operation and APInt allocation
while making the code more concise.
Signed-off-by: Ruoyu Qiu <cabbaken@outlook.com>
The Weak Zero SIV test concluded that there is a dependency only in the
`=`-direction when `Delta` is zero. This is incorrect, because the
coefficients of the addrecs might be zero, in which case the dependency
should have all directions. This patch adds non-zero check for the
coefficient to address the issue.
SCEV isKnownPredicate may crash if the expressions are involved with
different loops. To verify if two loops have the same iteration space,
we do not need to use the SCEV apis, and it can be done by the equality
check.
Moreover, no pass (not even loop fusion) requires to check SameSD levels
for more than one level. In this patch, we limit the analysis of SameSD
levels to only one level after the common levels.
Extract the logic to negate the dependence object from
`Dependence::normalize`. The extracted method will be used in the next
PR #185577 to refactor the Weak Zero SIV tests.
This patch adds a check to ensure that the addrecs have nsw flags at the
beginning of the Exact SIV test. If either of them doesn't have, the
analysis bails out. This check is necessary because the subsequent
process in the Exact SIV test assumes that they don't wrap.
This patch rewrites the formula in the Weak Zero SIV tests to match the
one used in the Strong SIV test that was updated in #179665. In this
form, `ConstantRange` is used so we don't need to pay attention to any
corner cases such as overflow.
Fix some test cases that were added in the past PRs to represent the
edge cases.
The symbolic RDIV test relies on computing the extremes of affine
expressions (e.g., `A1*N1` and `A2*N2`) to disprove dependencies. These
calculations were previously done using `SE->getMulExpr` and
`SE->getMinusSCEV` without guarding against signed integer overflow. If
large coefficients or loop bounds cause a wrap, `isKnownPredicate`
evaluates the wrapped values, potentially disproving a valid dependence
and leading to miscompilations.
This patch reimplements symbolicRDIVtest using `ConstantRange` to work
around overflows.
---------
Signed-off-by: Ruoyu Qiu <cabbaken@outlook.com>
Co-authored-by: Ryotaro Kasuga <kasuga.ryotaro@fujitsu.com>
Change the signature of `exactSIVtest` to directly pass addrecs instead
of passing their operands separately. I *think* this change is not
mandatory, but it will simplify the code, especially because we will be
checking the presence of nsw flags on the addrecs.
Change the signatures of the Weak Zero SIV tests, specifically by
passing an addrec directly instead of separating it into the coefficient
and constant. This form is more useful to address the several
correctness issues in those tests.
Replaces the `SmallVector`-based approach for computing the min/max of
affine domain bounds with `GetMaxOrMin` lambda returning `std::optional`
for better readability.
Previously, the code allocated a `SmallVector` to collect valid bounds
and relied on `smax(front(), back())` to handle the single-element case,
which may cause misunderstanding.
---------
Signed-off-by: Ruoyu Qiu <cabbaken@outlook.com>
Recently, the consistent flag and the peeling flags were removed from
the `Dependence` class (#181608, #183737). However, the related comments
were not deleted accordingly. This patch cleans them up.
Add overflow checks when computing `Delta` in the Weak Zero SIV tests.
The tests bail out if we cannot prove that the `Delta` computation does
not overflow. These calculations are also moved later so that some
analyses that do not require these checks can run first.
Fix part of the test cases added in #164246.
Currently Strong SIV test, does not check that the AddRecs involved do
not overflow. This is required for correctness of the tests. Strictly
speaking, the range-based independence check in Strong SIV relies on
SCEV which internally takes care of potential overflows, so this is
mainly needed for the divisibility test and distance/directions
calculations, but putting the test early in the function covers all the
cases anyways.
`isPeelFirst` and `isPeelLast` are updated only in the Weak Zero SIV
tests, and no clients actually use them. Keeping these features while
fixing the existing defects in DA would add unnecessary complexity. If
they are unnecessary in the first place, it would be better to delete
them to mitigate maintenance burden.
In the Weak Zero SIV tests, given two subscripts `{c0,+,a}` and `c1`,
when `c0 == c1`, the tests conclude that a dependency exists from the
former subscript at the first iteration to the latter subscript at every
iteration. However, this conclusion is correct only when `a` is not
zero, which was not being checked.
This patch adds non-zero checks for `a` in the Weak Zero SIV tests.
Fix the test cases added in #183735 .
The `Dependence` class has a flag `Consistent`, but it's not clear what
it represents. This flag doesn't seem to be updated correctly during
analysis. There are no users of it, so I think this flag would be
unnecessary.
In addition, because of this flag, even minor code changes can alter
test results unintentionally (specifically, the presence/absence of a
string "consistent" in the test output). Such test changes can be
confusing and gradually add to the maintenance burden.
Given these points, it's better to remove the `Consistent` flag from the
`Dependence` class entirely.
In the Strong SIV test, given two addrecs `{c0,+,a}` and `{c1,+,a}`, the
following inequality is evaluated:
`|c0 - c1| >s |a| * BTC`, where `BTC` is the backedge-taken count of the
loop.
To evaluate this correctly, at least the following checks are necessary.
- `c0 - c1` doesn't overflow
- For all absolute-value calculations `|x|`, `x` is not the signed
minimum value
- `|a| * BTC` doesn't overflow
- `0 <=s BTC`, which is currently missed
- The addrecs have `nsw`, which is also currently missed
Enumerating these conditions and inserting them one by one is risky, and
I believe it makes the software flaky, so it should be avoided. It's
also unclear if these conditions are sufficient.
This patch replaces the current implementation with one that uses
`ConstantRange` so that we don't need to check any condition by
ourselves.
Fixes the test case for the Strong SIV test added in #179664. I believe
a similar fix can be applied for other tests as well.
As mentioned in #179653, the "minor algebra" in `testRDIV` is incorrect.
This patch deletes that part completely.
Fixes the test case added in #179653.
`DependenceInfo::unifySubscriptType` is a function that takes two
subscripts and casts them to the wider type. Using this function can
sometimes lead to correctness issues, especially when combined with
`DependenceInfo::removeMatchingExtensions`, as in #148435. These two
functions are intended to broaden the scope of DA, but they can also
introduce correctness issues, mainly due to mishandling of `sext`/`zext`
and integer overflows.
To avoid these issues, this patch removes the `unifySubscriptType`
function. Currently, it has only one caller, which is part of the
validation logic for delinearization. Instead of calling
`unifySubscriptType`, this patch adds a type check and bails out if the
types do not match. Note that I'm not entirely sure whether there are
real cases where the types differ and the check is actually necessary.
Also, this patch doesn't include new test cases, as I have not found
concrete examples where `unifySubscriptType` itself causes actual
issues. That is, this patch may be NFC.
Fix#169807
If two subscripts has same cast operation (`sext` or `zext`),
`removeMatchingExtensions` strips off them. Due to the following
reasons, remaining this function is not useful and risky:
- If the operand of the cast operation has proper nowrap properties,
SCEV usually strips off the cast operation automatically. It's not very
meaningful to do it on DA side.
- If the operand doesn't have nowrap properties, stripping off the cast
operation may lead to incorrect result. Especially, if it can be
negative, removing `zext` would change the interpretation of the value
in a signed sense.
- This function is part of the root cause of #148435.
Although I've not found any cases where incorrect results are produced
by this function, but I think it doesn't make sense to keep it. As far
as I have checked, there are no regressions after removing this
function.
Resolve#169809
DA uses `DependenceInfo::isKnownPredicate` instead of
`ScalarEvolution::isKnownPredicate` in several places. The former is
intended to be a "wrapper" for the later. Specifically, it performs the
following processes:
- Replace `zext(X) cmp zext(Y)` with `X cmp Y`.
- Replace `X >=s Y` with `X - Y >=s 0`
- Replace `X <=s Y` with `X - Y <=s 0`
- Replace `X >s Y` with `X - Y >s 0`
- Replace `X <s Y` with `X - Y <s 0`
The first one can return an incorrect result when the most significant
bit of `X` and `Y` are different. Everything other than the first one
can be incorrect when `X - Y` overflows. Actually, when a `SCEVUnknown`
is involved (e.g., `%n <s %n + 1` will be `0 <s 1`), this function often
returns a result that ignore the possibility of overflow.
This patch removes `DependenceInfo::isKnownPredicate` and replace all
uses of it with `ScalarEvolution::isKnownPredicate`. There is a
degradation in some test cases, but this should be correct.
Resolve#169810
In Exact SIV and Exact RDIV tests, there are multiple `APInt` arithmetic
operations which can overflow. These overflows are currently not
checked, which may lead to incorrect analysis results. However, adding
overflow checks for each operation can clutter the code and make it
harder to read.
This patch introduces a new wrapper class `OverflowSafeSignedAPInt` that
encapsulates a `std::optional<APInt>` and provides arithmetic operations
with built-in overflow checking. If an arithmetic operation overflows,
the internal `std::optional<APInt>` is set to `std::nullopt`, indicating
an invalid result. Also, if any operand of an arithmetic operation is
invalid, the result will also be invalid. By using this wrapper class in
the Exact SIV and Exact RDIV tests, now overflows are handled properly
while keeping the readability of the code.
Fixes the test added in #171990.
Delinearization has its own `isKnownNonNegative` function, which wraps
`ScalarEvolution::isKnownNonNegative` and adds additional logic. The
additional logic is that, for a pointer addrec `{a,+,b}`, if the pointer
has `inbounds` and both `a` and `b` are known to be non-negative, then
the addrec is also known non-negative (i.e., it doesn't wrap). This
reasoning is incorrect. If the GEP and/or load/store using the pointer
are not unconditionally executed in the loop, then the addrec can still
wrap. Even though no actual example has been found where this causes a
miscompilation (probably because the subsequent checks fail so the
validation also fails), simply replacing it with
`ScalarEvolution::isKnownNonNegative` is safer, especially it doesn't
cause any regressions in the existing tests.
Resolve#169811
clean up interface of getIndexExpressionsFromGEP to get SCEV expressions
instead of int for Sizes of the arrays.
This intends to simplify the code in #156342 by avoiding conversions
from SCEV to int and back to SCEV.
Fix GitHub issue #149991 where Strong SIV test incorrectly concludes
'none!' for symbolic coefficients that could be zero, leading to 0/0
undef behavior.
The Strong SIV test was incorrectly concluding "no dependence" when the
coefficient is symbolic and the delta (difference between source and
destination) is zero.
When delta=0, the Strong SIV test divides delta/coeff to get the
distance.
The bug occurs when coeff is an unknown symbolic value: if coeff=0 at
runtime,
then 0/0 is undefined and all iterations access the same memory
location,
creating a true dependence that was being missed.
In `gcdMIVtest`, there is logic that assumes the addition(s) of
`SCEVAddExpr` don't overflow without any checks. Adding overflow checks
would be fine, but this part appeart to be less useful. So this patch
removes it.
Fix one of the tests added in #169926.
Removes DependenceInfo::getRuntimeAssumptions(), DependenceInfo::Assumptions,
and the print of "Runtime Assumptions:". The runtime assumptions are still
properly attached to each Dependence result and printed as part of the
per-dependence output.
This patch moves the validation logic of delinearization results from DA
to Delinearization. Also call it in `printDelinearization` to test its
behavior. The motivation is as follows:
- Almost the same code exists in `tryDelinearizeFixedSize` and
`tryDelinearizeParametricSize`. Consolidating it in Delinearization
avoids code duplication.
- Currently this validation logic is not well tested. Moving it to
Delinearization allows us to write regression tests easily.
This patch changes the test outputs and debug messages, but otherwise
NFCI.
This patch replaces the delinearization function used in DA, switching
from one that depends on type information in GEPs to one that does not.
There are three types of changes in regression tests: improvements,
degradations, and degradations but the related features will be
removed. Since there were very few cases that are classified into the
second category, I believe the impact of this change should be
practically insignificant.
This patch fixes the unexpected result in monotonicity check for
`@step_is_variant` in `monotonicity-no-wrap-flags.ll`. Currently, the
SCEV is considered non-monotonic if it contains an expression which is
neither loop-invariant nor an affine addrec. In `@step_is_variant`, the
`offset_i` satisfies this condition, but `offset_i + j` was classified
as monotonic.
The root cause is that a non-outermost loop was passed to monotonicity
checker instead of the outermost one. This patch ensures that the
correct outermost loop is passed.
Rely on the product of `UpperBound` and `AbsCoeff` only if SCEV
can prove that there is no overflow. Also the same about the result
of the subtraction of `DstConst` from `SrcConst` to calculate `Delta`.
Given a `SCEVMulExpr` such as `5 * %m`, `gcdMIVtest` in DA assumes the
value as a multiple of 5 in a mathematical sense. However, this is not
necessarily true if `5 * %m` overflows, especially because an odd number
has an inverse modulo `2^64`. Such incorrect assumptions can lead to
invalid analysis results.
This patch stops unconditionally extracting a constant operand from
`SCEVMulExpr`. Instead, it only allows this when the `SCEVMulExpr` has
the `nsw` flag.
