[PATCH] D22630: Loop rotation

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Gentle ping and a few comments. There are a few proprietary benchmarks that perform better with this pass and I'd like to see this (hopefully) moving towards a commit.



================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:225
           LoopID = MD;
+          I = TI;
+        }
----------------
hfinkel wrote:
> hiraditya wrote:
> > mzolotukhin wrote:
> > > What if a loop has two backedges? `LoopID` would be the same for both of them, but `TI` obviously would be different.
> > If multiple instructions are branching to the loop-header with via a back-edge, I think, that means there are two different loops.
> No, you can have a loop with multiple backedges. Backedges, by definition, go to the loop header. We have two functions in LoopInfo:
> 
>   /// If there is a single latch block for this loop, return it.
>   /// A latch block is a block that contains a branch back to the header.
>   BlockT *getLoopLatch() const;
> 
>   /// Return all loop latch blocks of this loop. A latch block is a block that
>   /// contains a branch back to the header.
>   void getLoopLatches(SmallVectorImpl<BlockT *> &LoopLatches) const {
>     ...
What does `getLoopID()` return today in the event of multiple latches? @hiraditya maybe it makes sense, for now at least, to `return { nullptr, nullptr };` when size of `GetLoopLatches() > 1`?


================
Comment at: llvm/lib/IR/Metadata.cpp:1311
+  const auto &Info = getContext().pImpl->InstructionMetadata.find(this)->second;
+  assert(!Info.empty() && "Shouldn't have called this");
+  Info.getAllIDs(MDIDs);
----------------
I'd prefer a message more like "No Instruction Metadata found".


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:62-64
+static cl::opt<int> MaxLoopsRotated(
+    "max-loops-rotated", cl::init(-1), cl::Hidden,
+    cl::desc("The maximum loops to be rotated (-1 means no limit)"));
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > How is this intended to be used?
> For delta-debugging. This patch actually exposed a bug in MachineBlockPlacement (https://llvm.org/bugs/show_bug.cgi?id=28604). where we needed this flag. I'll remove this once the patch is approved.
The bug is marked as fixed by a duplicate. Could you please remove and verify it is working?

FYI: https://bugs.llvm.org/show_bug.cgi?id=28104


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:207
+  const BranchInst *BI = dyn_cast<const BranchInst>(OrigH->getTerminator());
+  if (!BI || !BI->isConditional())
     return false;
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > Maybe I forgot something, but why exactly do we still need this check? I thought that one of the main benefits of this patch was to remove this limitation.
> If the loop-header has invoke instruction, or some other TerminatorInst, I'm not sure how to deal with them.
It'd be helpful to have a comment mentioning that you're not handling the unconventional exits like invoke.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:319
+          // Handle uses in the loop-closed-phi.
+          PHINode *ClosePhi = cast<PHINode>(UserInst);
+          BasicBlock *Pred = ClosePhi->getIncomingBlock(U.getOperandNo());
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > Why is UserInst guaranteed to be a PHINode?
> Because Inst does not dominate UserInst it must be the loop-closed-phi as the loop is in loop-closed-ssa form.
This also would be good to have as a comment. And possibly an `assert()` that validates the claim.


https://reviews.llvm.org/D22630