[PATCH] D119965: [LICM][PhaseOrder] Don't speculate in LICM until after running loop rotate

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/LICM.h:49
   unsigned LicmMssaNoAccForPromotionCap;
+  unsigned LicmAllowSpeculation;
 
----------------
bool


================
Comment at: llvm/include/llvm/Transforms/Scalar/LICM.h:68
   unsigned LicmMssaNoAccForPromotionCap;
+  unsigned LicmAllowSpeculation;
 
----------------
bool


================
Comment at: llvm/include/llvm/Transforms/Utils/LoopUtils.h:174-175
 /// Diagnostics is emitted via \p ORE. It returns changed status.
+/// \p AllowSpeculation is whether values should be hoisted even if they are not
+/// guaranteed to execute in the loop.
 bool hoistRegion(DomTreeNode *, AAResults *, LoopInfo *, DominatorTree *,
----------------
This sounds worse than it is.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:910
+    MPM.add(createLICMPass(LicmMssaOptCap, LicmMssaNoAccForPromotionCap,
+                           /*AllowSpeculation*/ true));
   }
----------------
Could we please be consisted with `/*AllowSpeculation=*/`/`/*AllowSpeculation*/`


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:206
+      unsigned LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap,
+      bool LicmAllowSpeculation = true)
+      : LoopPass(ID), LICM(LicmMssaOptCap, LicmMssaNoAccForPromotionCap,
----------------
Elsewhere `LicmAllowSpeculation` doesn't have a default value.
Should we be consistent with that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119965