[PATCH] D90290: [LoopInterchange] Prevent Loop Interchange for non-affine value store to affine access

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

I don't understand the reasoning here, even if I assume for a moment the underlying idea is sound, the patch doesn't seem to be.
For example, it will only check stores of a certain shape, why can we store anything to non-affine accesses? What about non-store accesses?
This looks too much like ad-hoc pattern matching.

The problem, as I see it, is not about "storing" but the fact that the loop carried dependence through `a` (caused by `a ^= 0x1000;`) is not taken into account properly. While by itself it looks like a reduction dependency (see for example [0]), it cannot be ignored as intermediate values escape. I haven't read all of loop interchange to determine where/why the dependency is broken, but that is the reason this breaks.

A sketch for an example without a "store of a non-affine value" would look something like this:

  void test_deps() {
      for (int i = 0; i <= 3; i++) {
          for (int j = 0; j <= 3; j++) {
              a ^= 0x1000;
              if (a == ...)
                c[j][i] = 42;
          }
      }
  }

[0] https://arxiv.org/abs/1505.07716



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1021
+      return true;
+  }
+  return false;
----------------
This breaks out of this loop with both true and false return value. That cannot be correct.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90290/new/

https://reviews.llvm.org/D90290