[PATCH] D75943: [LoopInterchange] Fix interchanging contents of preheader BBs

[Alexey Zhikhartsev via Phabricator via llvm-commits]

alexey.zhikhar marked 9 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:
> 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).
Oh, I see now. Fixed.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1335
+  SmallVector<Instruction *, 4> TempInstrs;
+  for (Instruction &I : *BB1) {
+    if (&I == BB1->getTerminator()) {
----------------
fhahn wrote:
> 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.
Thanks, I didn't know about these utils. I ended up with using map_range().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75943