[PATCH] D107281: [SimpifyCFG] Speculate a store preceded by a local non-escaping load

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

On the profitability of this transform, it looks like this is now always profitable given the restrictions and limiations of `isSafeToSpeculateStore`? Its description of  says:

  /// Determine if we can hoist sink a sole store instruction out of a
  /// conditional block.

and looking where this was checked I found:

  ++SpeculatedInstructions;
    if (SpeculatedInstructions > 1)

But perhaps it's good to check this and have another (negative) case for that.

Just checking if we haven't missed anything, did you ran benchmarks and didn't find any regressions related to this?



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2232
   // Look for a store to the same pointer in BrBB.
   unsigned MaxNumInstToLookAt = 9;
   // Skip pseudo probe intrinsic calls which are not really killing any memory
----------------
Unrelated, but nice magic number here.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2261
+        auto *AI = dyn_cast<AllocaInst>(getUnderlyingObject(StorePtr));
+        if (AI && !PointerMayBeCaptured(AI, false, true))
+          // Found a previous load, return it.
----------------
The description of `PointerMayBeCaptured` says its an expensive function, but with magic number 9 that `MaxNumInstToLookAt` is set to that doesn't seem to be an issue.


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

https://reviews.llvm.org/D107281