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.
The type sizes of backedge taken counts for two loops can be different
and this is to fix the crash in haveSameSD
(https://github.com/llvm/llvm-project/issues/165014).
---------
Co-authored-by: Shimin Cui <scui@xlperflep9.rtp.raleigh.ibm.com>
This patch fixes the computation of the absolute value for SCEV.
Previously, it was calculated as `AbsX = SE->isKnownNonNegative(X) ? X :
-X`, which would incorrectly assume that `!isKnownNonNegative` implies
`isKnownNegative`. This assumption does not hold in general, for
example, when `X` is a `SCEVUnknown` and it can be an arbitrary value.
To compute the absolute value properly, we should use
ScalarEvolution::getAbsExpr instead.
Fix#149977
PR #157084 added an option `da-run-siv-routines-only` to run only SIV
routines in DA. This PR replaces that option with a more fine-grained
one that allows to select other than SIV routines as well. This option
is useful for regression testing of individual DA routines. This patch
also reorganizes regression tests that use `da-run-siv-routines-only`.
The dependence testing functions in DA assume that the analyzed AddRec
does not wrap over the entire iteration space. For AddRecs that may
wrap, DA should conservatively return unknown dependence. However, no
validation is currently performed to ensure that this condition holds,
which can lead to incorrect results in some cases.
This patch introduces the notion of *monotonicity* and a validation
logic to check whether a SCEV is monotonic. The monotonicity check
classifies the SCEV into one of the following categories:
- Unknown: Nothing is known about the monotonicity of the SCEV.
- Invariant: The SCEV is loop-invariant.
- MultivariateSignedMonotonic: The SCEV doesn't wrap in a signed sense
for any iteration of the loops in the loop nest.
The current validation logic basically searches an affine AddRec
recursively and checks whether the `nsw` flag is present. Notably, it is
still unclear whether we should also have a category for unsigned
monotonicity.
The monotonicity check is still under development and disabled by
default for now. Since such a check is necessary to make DA sound, it
should be enabled by default once the functionality is sufficient.
Split off from #154527.
Bail out when the backedge taken count is a CouldNotCompute SCEV in
function isKnownLessThan; we cannot and do not want to query things like
its Type.
Fixes#159979
This is a follow-up PR for post-commit comments in #128782 .
- Fix variable name to camel case.
- Change the output format to make it easier to handle with FileCheck.
- Regenerate assertions of regression tests.
When building an assert-enabled target, silence the following:
```
C:\git\llvm-project\llvm\include\llvm/Analysis/DependenceAnalysis.h(290): warning C4018: '<=': signed/unsigned mismatch
```
When there is a dependency between two memory instructions in separate loops that have the same iteration space and depth, SIV will be able to test them and compute the direction and the distance of the dependency.
This patch adds an overflow check to the `exactSIVtest` function to fix
the issue demonstrated in the test case added in #157085. This patch
only fixes one of the routines. To fully resolve the test case, the
other functions need to be addressed as well.
This patch introduces a new option, `da-run-siv-routines-only`, which
runs only the SIV family routines in the DA. This is useful for testing
(regression tests, not dependence tests) as it helps detect behavioral
changes in the SIV routines. Actually, regarding the test cases added in
#157085, fixing the incorrect result requires changes across multiple
functions (at a minimum, `exactSIVtest`, `gcdMIVtest` and
`symbolicRDIVtest`). It is difficult to address all of them at once.
This patch also generates the CHECK directives using the new option for
`ExactSIV.ll` as it is necessary for subsequent patches. However, I
believe it will also be useful for other `xxSIV.ll` tests. Notably, the
SIV family routines tend to be affected by other routines, as they are
typically invoked at the beginning of the overall analysis.
