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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

Do note that the cycle hierarchy is implementation-defined, and hence the reported cycle depth is not an invariant property of the program being compiled. For example, in the following CFG:

       Entry
         |\
         v \
     .-> P  \
    /    |   \
   /     v    \
  '      Q     \
  |     / \     |
  |    /   \    |
  |   v     v   |
  `-- S <-- R <-'
      |
      v
     Exit

Here. the analysis can either identify one cycle with header P, or two nested cycles with headers R and S.

The change description says that MachineSink is "sensitive" to cycle depth. Is it only the performance of the resulting program, or does it affect correctness too?



================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:82
+
+  if (++(successors(Predecessor).begin()) != successors(Predecessor).end())
+    return nullptr;
----------------
Why not just use succ_size()? It's defined in CFG.h for LLVM IR and in MachineSSAContext.h for Machine IR.

Also, minor optimization ... this check is probably slightly cheaper than isLegalToHoistInto(), so it might help to check it earlier.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:141
+
+  bool isCycleInvariant(InstructionT &I) const;
+
----------------
This is not defined for LLVM IR. Maybe move it to the Machine IR specialization?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995