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

[Alexey Zhikhartsev via Phabricator via llvm-commits]

alexey.zhikhar marked 8 inline comments as done.
alexey.zhikhar added inline comments.


================
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.");
----------------
fhahn wrote:
> 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.
I saw them in LICM so I thought that there's no harm in adding them to loop interchange. When I first saw the error that this patch aims to fix, it was an assertion in LICM that runs after loop interchange, which was misleading.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1335
+  SmallVector<Instruction *, 4> TempInstrs;
+  for (Instruction &I : *BB1) {
+    if (&I == BB1->getTerminator()) {
----------------
fhahn wrote:
> I think this could just be something like 
> `SmallVector<Instruction *, 4> TempInstrs(BB1->begin(), std::prev(BB1->end()); // Save all non-terminator instructions.`
That would be nice but it doesn't seem to work, I'm getting `error: cannot convert ‘llvm::Instruction’ to ‘llvm::Instruction*’ in initialization`.
Instead, I replaced the for-each loop with explicit iterators and stop before when the terminator is encountered.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1341
+  }
+  for (Instruction *I : TempInstrs) {
+    I->removeFromParent();
----------------
fhahn wrote:
> nit: drop unneeded {
This is a small thing but it's something that I thought about before and I would appreciate your input. I personally prefer adding braces even when there's only one statement in a loop/if condition since this way it is both easier and less error-prone to add new statements. I also don't see the LLVM guidelines mentioning anything in this regard but, of course, I'll fix this nit if you insist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75943