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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, with the remaining comments, thanks!



================
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.");
----------------
alexey.zhikhar wrote:
> 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.
Yes, the assertions  after we do the transformations (lines 586 and 588) are very beneficial and should guard against similar error in the future. The ones here mean an earlier pass violated LCSSA and I think it would be slightly preferable to remove them; other passes are responsible for that (and -verify-lcssa can be used to catch those).


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1335
+  SmallVector<Instruction *, 4> TempInstrs;
+  for (Instruction &I : *BB1) {
+    if (&I == BB1->getTerminator()) {
----------------
alexey.zhikhar wrote:
> 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.
Ah right, this is unfortunate.for the iterator version, you could use something like `for (auto &I : make_range(BB->begin(), std::prev(BB-end)))`

If you use make_early_inc_range, it should be safe to merge the loops adding to TempInstrs and removing I.

Or you could use map_range to get an iterator range that returns Instruction * instead Instruction& like below.

```
+  auto Iter = map_range(*BB1, [](Instruction &I) { return &I });
+  SmallVector<Instruction *, 4> TempInstrs(Iter.begin(), std::prev(Iter.end()));
+  for (Instruction &I : TempInstrs)
+    I.removeFromParent();
```

Any of the options is fine by me.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1341
+  }
+  for (Instruction *I : TempInstrs) {
+    I->removeFromParent();
----------------
alexey.zhikhar wrote:
> 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.
I don't think this is explicitly spelled out in the coding standard, but not using unnecessary braces is *very very* common (at least in llvm/ from my experience). IMO this falls under `use the style that is already being used so that the source is uniform and easy to follow` which is mentioned in the beginning of https://llvm.org/docs/CodingStandards.html#golden-rule

As far as I'm aware, the existing code in LoopInterchange.cpp does not use trivial braces (and if it does it's probably by accident) and IMO it would be good for the sake of consistency to keep it like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75943