[PATCH] D123995: [MachineSink] replace MachineLoop with MachineCycle

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

LGTM but let's wait for @sameerds for accept.



================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:103
 
+  auto getHeaders() const -> const SmallVector<BlockT *, 1> & {
+    return Entries;
----------------
shchenz wrote:
> MatzeB wrote:
> > MatzeB wrote:
> > > as above
> > - Other functions are called `isEntry` and `printEntries` and the CycleTerminology documentation calls them entries as well. So this should be called `getEntries()` I think.
> > 
> > - Use `SmallVectorImpl<T>` without specifying the size for APIs 
> Thanks. I will commit another patch to rename above `getHeader()` later.
The header is define in `CycleTerminology.rst` as the first entry we find. So the name of the `getHeader` function seems fine to me. (The `getHeaders` here was questionable, because the documented hinted that there is only one and those things were already called entries anyway).


================
Comment at: llvm/include/llvm/CodeGen/MachineCycleAnalysis.h:45
+
+  // TODO: verify analysis
+};
----------------
shchenz wrote:
> MatzeB wrote:
> > Do we need a `TODO` comment here? Will the verify analysis added at this particular line?
> Moved from `lib/CodeGen/MachineCycleAnalysis.cpp`. I thought, it is a placeholder for a declare of verify function?
Can you just remove it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995