[DA] Add an option to control delinearization validity checks

[bmahjour]

Summary

Dependence Analysis performs static checks to confirm validity of delinearization. These checks often fail for 64-bit targets due to type conversions and integer wrapping that prevent simplification of the SCEV expressions. These checks would also fail at compile-time if the lower bound of the loops are compile-time unknown.

For example:

void foo(int n, int m, int a[][m]) {
  for (int i = 0; i < n; ++i)
    for (int j = 0; j < m; ++j) {
      a[i][j] = a[i+1][j-2];
    }
}

opt -mem2reg -instcombine -indvars -loop-simplify -loop-rotate -inline -pass-remarks=.* -debug-pass=Arguments  -da-permissive-validity-checks=false k3.ll -analyze -da

will produce the following by default:

da analyze - anti [* *|<]!

but will produce the following expected dependence vector if the validity checks are disabled:

da analyze - consistent anti [1 -2]!

This revision will introduce a debug option that will leave the validity checks in place by default, but allow them to be turned off. New tests are added for cases where it cannot be proven at compile-time that the individual subscripts stay in-bound with respect to a particular dimension of an array. These tests enable the option to provide user guarantee that the subscripts do not over/under-flow into other dimensions, thereby producing more accurate dependence vectors.

For prior discussion on this topic, leading to this change, please see the following thread: http://lists.llvm.org/pipermail/llvm-dev/2019-May/132372.html

[dmgreen]

As a debugging tool, this sounds fine. Not so sure about the tests, but I guess they are not hurting anyone :) And show good room for improvement.

Do you have any ideas how we might get the llvm form of them we have here (not the c form) to give good DA results without the miscompiles that the new option would produce? I was wondering if it was worth trying to get DA to return a dependency that is conditional on a set of predicates. Clients (we only have 3 at the moment) could treat predicated dependencies as "confused", or version the loop based on them.

[bmahjour]

In D62610#1523995, @dmgreen wrote:

As a debugging tool, this sounds fine. Not so sure about the tests, but I guess they are not hurting anyone :) And show good room for improvement.

Do you have any ideas how we might get the llvm form of them we have here (not the c form) to give good DA results without the miscompiles that the new option would produce? I was wondering if it was worth trying to get DA to return a dependency that is conditional on a set of predicates. Clients (we only have 3 at the moment) could treat predicated dependencies as "confused", or version the loop based on them.

The tests are adding a bit more coverage. Of all the tests currently available only three tests in DADelin.ll are checking for loop nests with consistent, non-scalar loop carried dependencies and those are all using 32-bit addressing mode and none has a runtime loop lower-bound.

I'm not sure having the predicated dependencies will make them more usable in practice, since loop versioning is highly undesirable (due to code bloat and introduction of new branches) especially as the number of transformations that rely on dependence analysis increase in the loop optimization pipeline.

[jdoerfert]

I would have liked to invert the option though. Most similar options are set up such that they allow to opt-out of the sound behavior.

[bmahjour]

In D62610#1526365, @jdoerfert wrote:

I would have liked to invert the option though. Most similar options are set up such that they allow to opt-out of the sound behavior.

Are you saying you'd like the validity checks to be off by default? Unfortunately for the cases mentioned by Dave and Michael, the compiler would have to be pessimistic and do these validity checks otherwise incorrect transformations can take place. That's why this option leaves the checks intact by default.

[jdoerfert]

In D62610#1527633, @bmahjour wrote:

In D62610#1526365, @jdoerfert wrote:

I would have liked to invert the option though. Most similar options are set up such that they allow to opt-out of the sound behavior.

Are you saying you'd like the validity checks to be off by default? Unfortunately for the cases mentioned by Dave and Michael, the compiler would have to be pessimistic and do these validity checks otherwise incorrect transformations can take place. That's why this option leaves the checks intact by default.

I do want validity checks by default.

I thought about having the option to be called something like "da-delinearization-no-checks". It would be false by default and it would avoid the "=false" part you need now. Setting it would mean you get the unsound behavior.

Now I don't feel strongly about it. Go ahead with the current wording if you want.

[fhahn]

In D62610#1529504, @jdoerfert wrote:

In D62610#1527633, @bmahjour wrote:

In D62610#1526365, @jdoerfert wrote:

I would have liked to invert the option though. Most similar options are set up such that they allow to opt-out of the sound behavior.

Are you saying you'd like the validity checks to be off by default? Unfortunately for the cases mentioned by Dave and Michael, the compiler would have to be pessimistic and do these validity checks otherwise incorrect transformations can take place. That's why this option leaves the checks intact by default.

I do want validity checks by default.

I thought about having the option to be called something like "da-delinearization-no-checks". It would be false by default and it would avoid the "=false" part you need now. Setting it would mean you get the unsound behavior.

I agree it would be better to have an option to actively disable the checks, rather than the other way around. ( personally I also think something like -da-disable-delinearization-checks would be a bit more descriptive). Also, I think it is worth calling out the unsoundness of setting the option in the description.

[jdoerfert]

In D62610#1529578, @fhahn wrote:

In D62610#1529504, @jdoerfert wrote:

In D62610#1527633, @bmahjour wrote:

In D62610#1526365, @jdoerfert wrote:

I would have liked to invert the option though. Most similar options are set up such that they allow to opt-out of the sound behavior.

Are you saying you'd like the validity checks to be off by default? Unfortunately for the cases mentioned by Dave and Michael, the compiler would have to be pessimistic and do these validity checks otherwise incorrect transformations can take place. That's why this option leaves the checks intact by default.

I do want validity checks by default.

I thought about having the option to be called something like "da-delinearization-no-checks". It would be false by default and it would avoid the "=false" part you need now. Setting it would mean you get the unsound behavior.

I agree it would be better to have an option to actively disable the checks, rather than the other way around. ( personally I also think something like -da-disable-delinearization-checks would be a bit more descriptive). Also, I think it is worth calling out the unsoundness of setting the option in the description.

+1

[bmahjour]

Renamed the option to -da-disable-delinearization-checks and set the default value to "false". Updated LIT tests accordingly.

[bmahjour]

formatting change to reduce the number of new lines in the options description.

[jdoerfert]

LGTM

llvm/trunk/lib/Analysis/DependenceAnalysis.cpp

 STATISTIC(GCDindependence, "GCD independence");
 STATISTIC(BanerjeeApplications, "Banerjee applications");
 STATISTIC(BanerjeeIndependence, "Banerjee independence");
 STATISTIC(BanerjeeSuccesses, "Banerjee successes");
 
 static cl::opt<bool>
     Delinearize("da-delinearize", cl::init(true), cl::Hidden, cl::ZeroOrMore,
                 cl::desc("Try to delinearize array references."));
+static cl::opt<bool> DisableDelinearizationChecks(
+    "da-disable-delinearization-checks", cl::init(false), cl::Hidden,
+    cl::ZeroOrMore,
+    cl::desc(
+        "Disable checks that try to statically verify validity of "
+        "delinearized subscripts. Enabling this option may result in incorrect "
+        "dependence vectors for languages that allow the subscript of one "
+        "dimension to underflow or overflow into another dimension."));
 
 //===----------------------------------------------------------------------===//
 // basics
 
 DependenceAnalysis::Result
 DependenceAnalysis::run(Function &F, FunctionAnalysisManager &FAM) {
   auto &AA = FAM.getResult<AAManager>(F);
   auto &SE = FAM.getResult<ScalarEvolutionAnalysis>(F);
bool DependenceInfo::tryDelinearize(Instruction *Src, Instruction *Dst,
   int size = SrcSubscripts.size();
 
   // Statically check that the array bounds are in-range. The first subscript we
   // don't have a size for and it cannot overflow into another subscript, so is
   // always safe. The others need to be 0 <= subscript[i] < bound, for both src
   // and dst.
   // FIXME: It may be better to record these sizes and add them as constraints
   // to the dependency checks.
+  if (!DisableDelinearizationChecks)
     for (int i = 1; i < size; ++i) {
       if (!isKnownNonNegative(SrcSubscripts[i], SrcPtr))
         return false;
 
       if (!isKnownLessThan(SrcSubscripts[i], Sizes[i - 1]))
         return false;
 
       if (!isKnownNonNegative(DstSubscripts[i], DstPtr))
         return false;
 
       if (!isKnownLessThan(DstSubscripts[i], Sizes[i - 1]))
         return false;
     }
 
   LLVM_DEBUG({
     dbgs() << "\nSrcSubscripts: ";
     for (int i = 0; i < size; i++)
       dbgs() << *SrcSubscripts[i];
     dbgs() << "\nDstSubscripts: ";
     for (int i = 0; i < size; i++)
       dbgs() << *DstSubscripts[i];