[PATCH] D105802: [LoopFlatten] Fix missed LoopFlatten opportunity

[Rosie Sumpter via Phabricator via llvm-commits]

RosieSumpter added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:178
+      Increment = dyn_cast<BinaryOperator>(LatchValue);
+      ConstantInt *RHS = cast<ConstantInt>(Compare->getOperand(1));
+      ConstantInt *One = ConstantInt::get(RHS->getType(), 1, true);
----------------
SjoerdMeijer wrote:
> I am wondering what happens if the RHS is not a constant, so instead of this:
> 
>   %cmp = icmp ult i32 %j, 18
> 
> we have something like:
> 
>     %cmp = icmp ult i32 %j, %N
> 
> Do we reject that earlier, and/or do we not have these test cases yet?
It shouldn't get here unless it is constant since this only catches the case where the compare which is the condition of the back edge has been changed from e.g. 
```
%cmp = icmp ult i32 %inc, 19  
``` 
to 
```
%cmp = icmp ult i32 %j, 18  
```
If the RHS is unknown e.g.

```
%cmp = icmp ult i32 %inc, %N
```
it won't have been changed and so will be caught by one of the previous pattern matches. 

I've altered the comments so that it's clearer that this match is only for the case when limit is a constant, and also added an assert to ensure that the LHS of the compare is the induction phi.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:181
+      Limit = ConstantInt::get(Compare->getContext(),
+                               RHS->getValue() + One->getValue());
+    }
----------------
SjoerdMeijer wrote:
> And I was also wondering if we need to worry about RHS being INT_MAX, in which case the + 1 will overflow.
So here the limit is RHS+1 because the another transformation changed the compare from 
```
%cmp = icmp ult i32 %inc, limit  
``` 
to 
```
%cmp = icmp ult i32 %j, limit-1  
```
(where limit is a known constant) so the only way RHS=limit-1 would be INT_MAX is if limit had already overflowed, so I think we don't need to check for it here?


================
Comment at: llvm/test/Transforms/LoopFlatten/loop-flatten-negative.ll:348
+
+; test_10: If the step is not 1, the loop shouldn't be flattened.
+define i32 @test_10(i32* nocapture %A) {
----------------
SjoerdMeijer wrote:
> You need to `CHECK:` something here.
The only CHECK in this file is at the beginning:
```
CHECK-NOT: Checks all passed, doing the transformation
```
which checks that all these tests fail doesn't it? Or should I being doing more specific checks here?


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

https://reviews.llvm.org/D105802