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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:174
       SE->getTripCountFromExitCount(SE->getBackedgeTakenCount(L));
   if (SE->getSCEV(TripCount) != SCEVTripCount) {
     if (!IsWidened) {
----------------
Nit: every time I need to read this `if` a couple of times, but I think it makes sense. Just for readability, to reduce indentation inside the if a little bit, I was wondering if this would help:

  if (SE->getSCEV(TripCount) != SCEVTripCount) && !IsWidened) {
  ..
  } else if (SE->getSCEV(TripCount) != SCEVTripCount)) {
  ..
  }


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:181
+      }
+      ConstantInt *One = ConstantInt::get(RHS->getType(), 1, true);
+      TripCount = ConstantInt::get(TripCount->getContext(),
----------------
Nit: what was this `true` argument again? I would guess thought that we don't need...


================
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) {
----------------
RosieSumpter wrote:
> 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?
Ah, sorry, I had missed that. It's indeed fine like this.


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

https://reviews.llvm.org/D105802