[PATCH] D88880: [LoopFlatten] Initial support for different types

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:386
+      //
+      // For the overflow check, we first calculate an 'effective' GEP
+      // bitwidth. I.e., we look through a ZExt instruction which widens the
----------------
ostannard wrote:
> I don't think this logic is correct. The check below is proving that the add and mul cannot overflow because the GEP would overflow before they do, and that would be UB. However, in the example in this comment, the add or mul could overflow, but the GEP would be fine, because it never sees an offset greater than 2*32-1. Since there's no UB, we would need to preserve the wrapping behaviour, which we can't do with a single, simple loop.
Actually, I think the original version of this is incorrect too:

  extern char a[];

  #define SIZE 100000

  int first = 1;

  void foo(unsigned lim) {
    for (unsigned i = 0; i < lim; ++i) {
      for (unsigned j = 0; j < SIZE; ++j) {
        // This might overflow if lim is large, but that is well-defined.
        unsigned x = i * SIZE + j;
        if (first) {
          // Access memory using the computed index. Only do this the first time,
          // so the address calculation won't overflow.
          asm volatile("" : : "r" (a[x]));
          first = 0;
        }
        // Use the computed index for something which isn't UB.
        asm volatile("" : : "r" (x));
      }
    }
  }

This gets flattened because the GEP check below passes, but the GEP is only actually executed once, and doesn't overflow the address space. I think everything in the source is well-defined, but flattening it changes the number of times the second asm statement is executed.

We might be able to salvage this by adding a check that the GEP dominates the loop latch?


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

https://reviews.llvm.org/D88880