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

Thu Oct 27 13:42:27 PDT 2022

Narutoworld marked 8 inline comments as done.
Narutoworld added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:557
 public:
   LoopFuser(LoopInfo &LI, DominatorTree &DT, DependenceInfo &DI,
             ScalarEvolution &SE, PostDominatorTree &PDT,
----------------
aeubanks wrote:
> I guess `DependenceInfo` doesn't need to be updated because it has no state
@aeubanks Thanks for the comment. 

I think the `DependenceInfo` does not need to be updated, since it operates on instructions which are not modified by `loopSimplify` pass.
I remember I found an example in the codebase. 

================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:599
+          if (!L->isLoopSimplifyForm())
+            Changed |= simplifyLoop(L, &DT, &LT, &SE, &AC, nullptr,
+                                    /*PreserveLCSSA=*/false);
----------------
bjope wrote:
> 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.
@bjope Thanks for your comment.
I totally agree with your suggestion, just update the patch. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136781