[PATCH] D66334: [LoopUnroll] Handle certain PHIs in full unrolling properly.

Mon Aug 19 05:53:06 PDT 2019

fhahn added a comment.

It would be great if you could add another function to the test, where we do not fully unroll the loop, i.e. by making sure it has a high trip count. For that, the minsize/optsize attributes need to be removed. Also, could you change the test file name to something a bit more descriptive, like `unroll-header-exiting-with-phis.ll`?

================
Comment at: test/Transforms/LoopUnroll/full-unroll-miscompile.ll:38
+entry:
+  %tmp = load i16, i16* @a, align 1
+  %tmp1 = load i16, i16* getelementptr inbounds ([2 x i16], [2 x i16]* @c, i32 0, i32 0), align 1
----------------
ebevhan wrote:
> fhahn wrote:
> > Can you strip away the unnecessary stores/loads? They do not impact unrolling.
> What should I replace the sdiv with in that case? Just a single load?
it's only used in %.lcssa10, you can replace it with an arbitrary constant there.

Instead of %tmp2 outside the loop, it would be good to have a loop dependent load or something, e.g. (simply pass an i16* %A as argument to the function).

```
  %ptr = getelementptr inbounds i16, i16* %A, i64 %i.0
  %tmp2 = load i16, i16* %ptr, align 1
```

================
Comment at: test/Transforms/LoopUnroll/full-unroll-miscompile.ll:47
+  %.lcssa10 = phi i16 [ %div, %entry ], [ %.lcssa, %for.cond.cleanup3 ]
+  %i.0 = phi i64 [ 37, %entry ], [ %inc9, %for.cond.cleanup3 ]
+  %cmp = icmp ult i64 %i.0, 38
----------------
Another nit: we could make %i.0 start at 0 and adjust the check in %cmp accordingly. Maybe also change it so we have more than 1 iteration to unroll, e.g. 2 or 3 iterations.

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

https://reviews.llvm.org/D66334