[PATCH] D36783: [x86] Refactor the CMOV conversion pass to be more flexible.

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added a comment.

The code looks good, just few comments need to be fixed/added (see below)



================
Comment at: lib/Target/X86/X86CmovConversion.cpp:100
   ///
   /// \param CurrLoop Loop being processed.
   /// \param CmovInstGroups List of consecutive CMOV instructions in CurrLoop.
----------------
need to fix the "param" comment.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:109
   ///
   /// \param CurrLoop Loop being processed.
   /// \param CmovInstGroups List of consecutive CMOV instructions in CurrLoop.
----------------
need to fix the "param" comment.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:169
+  SmallVector<MachineLoop *, 4> Loops(MLI.begin(), MLI.end());
+  for (int i = 0; i < (int)Loops.size(); ++i)
+    for (MachineLoop *Child : Loops[i]->getSubLoops())
----------------
You are calling "Loops.size()" every iteration intentionally, as you want to handle the just new added sub-loops.
Do not you think we should comment on that, so nobody will optimize this by saving the initial "size()" once and check against it later?


https://reviews.llvm.org/D36783