[PATCH] D136781: ensure loop-simplifed form when running loop-fusion pass with new-PM

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:599
+          if (!L->isLoopSimplifyForm())
+            Changed |= simplifyLoop(L, &DT, &LT, &SE, &AC, nullptr,
+                                    /*PreserveLCSSA=*/false);
----------------
aeubanks wrote:
> `LI`?
Is this safe. Afaik simplifyLoop might create new sub-loops. So I wonder if it wouldn't be safer to simplify loops before creating the LDT, to ensure the LDT include all simplified loops (and ensure that the LDT isn't having stale entries now when the loop has been transformed (not really sure if that can happen, but if doing all simplification first I wouldn't need to bother about knowing such things)).

Another thing is that this code is shared between the Legacy PM implementation and the new PM implementation. But since the legacy PM has required LoopSimplify before the pass we do not really need to bother about it here.

So I think it could be better to take care of enforcing loop simplify form in LoopFusePass::run instead. Before creating the LoopFuser.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:602
+        }
+        if (Changed)
+          PDT.recalculate(F);
----------------
I guess you only need to recalculate if simplifyLoop returned true this iteration of the loop over LDT. The Changed variable however is the accumulated state (so that one could be true even if no loops have been simplified, but rather we fused some loops in an earlier iteration).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136781