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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 2 inline comments as done.
shchenz added a comment.

> I was concerned about compile-time impact of using MachineCycleInfo, but it looks like the impact is fairly low on CTMark: http://llvm-compile-time-tracker.com/compare.php?from=42865819b22486963294434fb21b51ab3e6ebfa4&to=61706f3ee1c979e7ec893d0bc4858790561a27c0&stat=instructions Looks like SPASS with ReleaseLTO-g is the only one with some non-trivial impact.

Thanks very much for the testing @nikic and glad to hear that the compile time impact is low. Let me know if you think the %0.39 improve for SPASS needs to be further investigated.

> 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?

I think the depth check should only impact the performance in machinesink. However there are some codes(`hasStoreBetween`) which does not work right for irreducible loops, with this patch, we can get correct behavior. But I think we need more investigation for that function. I plan to do more investigation later to avoid errors.



================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:82
+
+  if (++(successors(Predecessor).begin()) != successors(Predecessor).end())
+    return nullptr;
----------------
sameerds wrote:
> 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.
Thanks, updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995