[PATCH] D75943: [LoopInterchange] Do not break LCSSA

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the patch! It mostly looks good, just a few small comments inline.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:559
 
+    assert(InnerLoop->isLCSSAForm(*DT) && "Inner loop is not in LCSSA form.");
+    assert(OuterLoop->isLCSSAForm(*DT) && "Outer loop is not in LCSSA form.");
----------------
The assertions here might be a bit over-conservative. I think isLCSSAForm can potentially be expensive, so I would be slightly in favor of dropping them.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1335
+  SmallVector<Instruction *, 4> TempInstrs;
+  for (Instruction &I : *BB1) {
+    if (&I == BB1->getTerminator()) {
----------------
I think this could just be something like 
`SmallVector<Instruction *, 4> TempInstrs(BB1->begin(), std::prev(BB1->end()); // Save all non-terminator instructions.`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1341
+  }
+  for (Instruction *I : TempInstrs) {
+    I->removeFromParent();
----------------
nit: drop unneeded {


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1349
+  // Move instructions from TempInstrs to BB2.
+  for (Instruction *I : TempInstrs) {
+    I->insertBefore(BB2->getTerminator());
----------------
nit: drop unneeded {


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1620
   BasicBlock *InnerLoopPreHeader = InnerLoop->getLoopPreheader();
-  BasicBlock *OuterLoopHeader = OuterLoop->getHeader();
-  BranchInst *InnerTermBI =
-      cast<BranchInst>(InnerLoopPreHeader->getTerminator());
-
-  // These instructions should now be executed inside the loop.
-  // Move instruction into a new block after outer header.
-  moveBBContents(InnerLoopPreHeader, OuterLoopHeader->getTerminator());
-  // These instructions were not executed previously in the loop so move them to
-  // the older inner loop preheader.
-  moveBBContents(OuterLoopPreHeader, InnerTermBI);
+  swapBBContents(OuterLoopPreHeader, InnerLoopPreHeader);
 }
----------------
With adjustLoopPreheaders being so short now, please inline it directly into adjustLoopLinks.


================
Comment at: llvm/test/Transforms/LoopInterchange/lcssa-preheader.ll:1
+; RUN: opt < %s -basicaa -loop-interchange -pass-remarks-missed='loop-interchange' -verify-loop-lcssa -pass-remarks-output=%t -S
+; RUN: FileCheck --input-file %t --check-prefix REMARK %s
----------------
Please update the test to check the IR to ensure the pre-headers are swapped correctly. It would probably be good to check the whole structure. I think you should be able to also reduce the inner loop body a bit (one load/store should be enough).


================
Comment at: llvm/test/Transforms/LoopInterchange/lcssa-preheader.ll:5
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Please drop the target triple. Otherwise the test will fail when LLVM is built without the X86 backend. And it should not be required for the test.


================
Comment at: llvm/test/Transforms/LoopInterchange/lcssa-preheader.ll:17
+; REMARK-NEXT: lcssa_08
+; Function Attrs: nounwind
+define dso_local void @lcssa_08(i32** nocapture readonly %res, i32 %n, i32 %m) local_unnamed_addr #0 {
----------------
not needed.


================
Comment at: llvm/test/Transforms/LoopInterchange/lcssa-preheader.ll:18
+; Function Attrs: nounwind
+define dso_local void @lcssa_08(i32** nocapture readonly %res, i32 %n, i32 %m) local_unnamed_addr #0 {
+entry:
----------------
please drop the un-needed things like dso_local, local_unnamed_addr and #0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75943