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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

conditional store promotion is one of those transforms which I've been thinking about for a long time, and have never quite felt safe implementing.

My high level concern here is profitability.  It's unclear to me that inserting a flag iv, and the conditional store outside the loop is generally profitable.  It's clearly profitable in many cases, but I'm a bit hesitant on whether it makes sense to do as a canonicalization.  I'd love to see some data on the impact of this.

Structure wise, I strongly encourage you to use predicated stores not CFG manipulation.  LICM does not generally modify the CFG; we can, but the analysis updates are complicated.  (See the diamond hoisting code.)   I generally think that a predicate store instruction is the "right" construct here as it leads to more obvious optimization outcomes.

However, our masked.load handling for scalar (e.g. single value vectors) leaves a bit to be desired.  I'd started improving it a bit (with this goal in mind), but if you're serious about this, you'd need to spend some time working through related issues there first.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1924
       // FIXME: true for safety, false may still be correct.
+      if (PromoteConditionalAccesses) {
+        Value *FlagValue = FlagSSAUpdater->GetValueInMiddleOfBlock(ExitBlock);
----------------
See macro comment.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2103
+  const bool ShouldPromoteConditionalAccesses =
+      SetLicmConditionalAccessPromotion && loopHasNoAbnormalExits(CurLoop);
+
----------------
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.  


================
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
----------------
Stray change?


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


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2184
+              SawConditionalLIStore = true;
+              ConditionalLIStores.push_back(Store);
+              Alignment = std::max(Alignment, InstAlignment);
----------------
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.


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


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

https://reviews.llvm.org/D115244