[PATCH] D115244: [LICM] Promote conditional, loop-invariant memory accesses to scalars

[Dimitrije Milošević via Phabricator via llvm-commits]

dmilosevic141 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2103
+  const bool ShouldPromoteConditionalAccesses =
+      SetLicmConditionalAccessPromotion && loopHasNoAbnormalExits(CurLoop);
+
----------------
reames wrote:
> The abnormal exit property applies to classic store promotion too.  This is either redundant, or you have some bug you should discuss and fix separately.  
Thanks for pointing this out! I implemented the //loopHasNoAbnormalExits// function because I was unsure whether the //LICM// pass makes sure there are no abnormal loop exits before it does 'classic' (non-conditional) promotions. There was no difference before and after calling this function (i.e. no bugs which this call 'fixes'), except for the (now) unnecessary overhead.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2156
           }
-      } else if (const StoreInst *Store = dyn_cast<StoreInst>(UI)) {
+      } else if (StoreInst *Store = dyn_cast<StoreInst>(UI)) {
         // Stores *of* the pointer are not interesting, only stores *to* the
----------------
reames wrote:
> Stray change?
Just something that I needed for the set that I was using. Reverting this back.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2182
+                     (!SawConditionalLIStore || (InstAlignment > Alignment))) {
+            if (CurLoop->isLoopInvariant(Store->getPointerOperand())) {
+              SawConditionalLIStore = true;
----------------
reames wrote:
> This was already checked above.
Indeed, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2184
+              SawConditionalLIStore = true;
+              ConditionalLIStores.push_back(Store);
+              Alignment = std::max(Alignment, InstAlignment);
----------------
reames wrote:
> This set does not make sense.  A store only needs to be condition if we can't find any store to the address which doesn't dominate all exits.
> 
> You should only need to track some global state here, not anything store specific.
This set was used to propagate the value of the flag through the basic blocks, via the //SSAUpdater//'s interface.
For that purpose, we already had the //LoopUses// set, so this set was redundant. Thanks!


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2292
+    FlagSSAUpdater.Initialize(
+        Int8Ty,
+        ConditionalLIStores[0]->getPointerOperand()->getName().str() + ".flag");
----------------
reames wrote:
> Please use i1 here, not i8.  
Thanks!


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

https://reviews.llvm.org/D115244