[DA] remove constraint propagation and coupled/separable subscripts

[sebpop]

Remove all constraint propagation functions. Simplify depends() and
getSplitIteration() to test subscripts individually.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Sebastian Pop (sebpop)

Changes

Add -da-enable-constraint-propagation flag (disabled by default).

---

Full diff: https://github.com/llvm/llvm-project/pull/160924.diff

3 Files Affected:

• (modified) llvm/lib/Analysis/DependenceAnalysis.cpp (+8-2)
• (modified) llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll (+1-1)
• (modified) llvm/test/Analysis/DependenceAnalysis/Separability.ll (+1-1)

diff --git a/llvm/lib/Analysis/DependenceAnalysis.cpp b/llvm/lib/Analysis/DependenceAnalysis.cpp
index da86a8d2cc9c0..af1884048752b 100644
--- a/llvm/lib/Analysis/DependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/DependenceAnalysis.cpp
@@ -121,6 +121,10 @@ static cl::opt<unsigned> MIVMaxLevelThreshold(
     cl::desc("Maximum depth allowed for the recursive algorithm used to "
              "explore MIV direction vectors."));
 
+static cl::opt<bool> EnableConstraintPropagation(
+    "da-enable-constraint-propagation", cl::init(false), cl::Hidden,
+    cl::desc("Enable constraint propagation in dependence analysis."));
+
 //===----------------------------------------------------------------------===//
 // basics
 
@@ -3918,7 +3922,8 @@ DependenceInfo::depends(Instruction *Src, Instruction *Dst,
           for (unsigned SJ : Mivs.set_bits()) {
             // SJ is an MIV subscript that's part of the current coupled group
             LLVM_DEBUG(dbgs() << "\tSJ = " << SJ << "\n");
-            if (propagate(Pair[SJ].Src, Pair[SJ].Dst, Pair[SJ].Loops,
+            if (EnableConstraintPropagation &&
+                propagate(Pair[SJ].Src, Pair[SJ].Dst, Pair[SJ].Loops,
                           Constraints, Result.Consistent)) {
               LLVM_DEBUG(dbgs() << "\t    Changed\n");
               ++DeltaPropagations;
@@ -4233,7 +4238,8 @@ const SCEV *DependenceInfo::getSplitIteration(const Dependence &Dep,
       // propagate, possibly creating new SIVs and ZIVs
       for (unsigned SJ : Mivs.set_bits()) {
         // SJ is an MIV subscript that's part of the current coupled group
-        if (!propagate(Pair[SJ].Src, Pair[SJ].Dst, Pair[SJ].Loops, Constraints,
+        if (!EnableConstraintPropagation ||
+            !propagate(Pair[SJ].Src, Pair[SJ].Dst, Pair[SJ].Loops, Constraints,
                        Result.Consistent))
           continue;
         Pair[SJ].Classification = classifyPair(
diff --git a/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll b/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll
index e5d5d21e365a1..d03b797dc394f 100644
--- a/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/NonCanonicalizedSubscript.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
-; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa -da-enable-constraint-propagation 2>&1 \
 ; RUN: | FileCheck %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
diff --git a/llvm/test/Analysis/DependenceAnalysis/Separability.ll b/llvm/test/Analysis/DependenceAnalysis/Separability.ll
index 2ed9cca4d1fc0..3556d877e1dc0 100644
--- a/llvm/test/Analysis/DependenceAnalysis/Separability.ll
+++ b/llvm/test/Analysis/DependenceAnalysis/Separability.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
-; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa -da-enable-constraint-propagation 2>&1 \
 ; RUN: | FileCheck %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"

[kasuga-fj]

I think this feature should be removed, not just disabled behind a flag, for the following reasons:

• I don't think it will ever be re-enabled due to its correctness issues. Fixing those would require non-trivial changes.
• This feature doesn't seem particularly useful, as you can see almost no regression tests are affected, and notably Propagating.ll remains unchanged.
• It adds a fair amount of code complexity, making it harder to reason about other parts of DA. For instance, without it, we might no longer need to distinguish between Separable and Coupled.
• Fixing the correctness issues would require limiting its current capabilities, which would make it even less useful.

Given these points, I don't think it's worth keeping. The maintenance cost would likely outweigh any potential benefit.

[sebpop]

Ok, let's remove it.
If in the future we will need it we can always pick up bits and pieces from it.
I will send out a patch to remove constraint propagation and the separable subscripts printing.

[sebpop]

We rarely (maybe less than 1% of codes) need the Banerjee MIV test if delinearization is enabled.
Overall these patches remove 1900 SLoC and disable MIV another ~200 SLoC.
With these changes, DA is now about 3500 SLoC.

[sebpop]

Another cleanup is to move delinearization bits to Delinearize.cpp and that cuts another 200 SLoC.

[kasuga-fj]

To be clear: I am happy with this direction -- it's an alternative to NewDA that I proposed on Discourse. However, I'm not sure how things should proceed in this case. For example, should we submit a separate RFC to propose removing certain features from DA? Especially if we move forward with this way, there are other parts I'd also like to remove.

[sjoerdmeijer]

I have an admin question first: is this there an issue for this in the bug tracker? I am only asking because we have quite a few DA issues sitting in there now and I was trying to get an overview, and was wondering if this patch addresses one.

[sjoerdmeijer]

Ok, let's remove it.
If in the future we will need it we can always pick up bits and pieces from it.
I will send out a patch to remove constraint propagation and the separable subscripts printing.

I was going to suggest that putting it under an option would be the polite thing to do. It could be considered a deprecation step for downstream users. This would be the best thing to do if it was easy to compartmentalise the feature, but looking at the code, that might not be so straightforward. Correctness comes first, so I think that makes it okay to remove this feature.

To be clear: I am happy with this direction -- it's an alternative to NewDA that I proposed on Discourse. However, I'm not sure how things should proceed in this case. For example, should we submit a separate RFC to propose removing certain features from DA? Especially if we move forward with this way, there are other parts I'd also like to remove.

I don't think we need another RFC, maybe an update to the NewDA RFC would be appropriate.

[kasuga-fj]

I have an admin question first: is this there an issue for this in the bug tracker? I am only asking because we have quite a few DA issues sitting in there now and I was trying to get an overview, and was wondering if this patch addresses one.

If you're referring to GitHub issues, I think the answer is No. There are several potential issues in DA that I haven't submitted issues, mainly because I don't have any reproducers. The constraint propagation is one example. While I'm not planning to file such issues in the future either, I can share the potentially problematic parts I've noticed, if that would be helpful.

[sebpop]

I can share the potentially problematic parts I've noticed, if that would be helpful.

Yes, please. Make those comments in bug reports: it can be as short as "function() has those lines of code that seem to be wrong in version <sha_key> at <line_numbers>" and we will work on fixing those.

[sebpop]

To be clear: I am happy with this direction -- it's an alternative to NewDA

I'm happy that we can find a middle ground where we can focus on moving forward.

[sebpop]

I was going to suggest that putting it under an option would be the polite thing to do.

I agree, however the amount of code constraint propagation needs under the current implementation is ~2000 SLoC which is half the size of the rest of DA.

Constraint propagation simplifies the MIV test based on info inferred from SIV tests.
See Section 5.3 in https://softlib.rice.edu/pub/CRPC-TRs/reports/CRPC-TR90103-S.pdf and the running example:

for i
  for j
    A(i+1, i+j) = A(i, i+j)

Where the first SIV subscript i0+1 vs. i1 gets distance -1 propagated into the second MIV subscript i0+j0 vs. i1+j1 that then becomes SIV under the new constraints "j0 - 1" vs. "j1" which solves faster than the original MIV test.

[sjoerdmeijer]

I have looked a bit closer at the patch now, it looks like we are doing two things with this patch:

• disabling of constraint the propagation,
• disabling of the MIV test.

Why are we disabling the MIV test? Is that less effective or more difficult if we don't propagate the constraints?

[kasuga-fj]

This patch does at least the following three things:

• Removing constraint propagation
• Removing getSplitIteration
• Disabling banerjeeMIV by default

Can you please separate out these changes into separate PRs? I think the easiest first step is to remove getSplitIteration.

[kasuga-fj]

I can share the potentially problematic parts I've noticed, if that would be helpful.

Yes, please. Make those comments in bug reports: it can be as short as "function() has those lines of code that seem to be wrong in version <sha_key> at <line_numbers>" and we will work on fixing those.

This is a list of bugs I remembered for now, which I don't think I have been clearly stated anywhere. Please also refer to my discourse post.

Where	Why
unifySubscriptType	Maybe this should be applied recursively to an AddRec, after we implement the appropriate handling for wrap of an AddRec
removeMatchExtensions	Removing zext can change the result (consider the value of zext(x) when x is a negative value)
DependenceInfo::isKnownPredicate	Same as above, probably we are not allowed to purge zext in general. Also, it's unclear what happens if getMinusSCEV wraps
DependenceInfo::isKnownNonNegative	The inference from inbounds here is incorrect (cf. #160912)
DependenceInfo::isKnownLessThan	At the very least, calling getTruncateOrZeroExtend here is incorrect
findBoundsXX (e.g., findBoundsLT)	The equations in the comment above each function (following "Wolfe gives the equations") don't appear to consider overflow