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

[Momchil Velikov via Phabricator via llvm-commits]

chill added a comment.

In D107281#2921731 <https://reviews.llvm.org/D107281#2921731>, @lebedev.ri wrote:

> Please add a negative test that you believe showcases the problem the `PointerMayBeCaptured()` is solving.

Will do.

In D107281#2921750 <https://reviews.llvm.org/D107281#2921750>, @SjoerdMeijer wrote:

> 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.

OK.

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

I've got these results (`1 - Time_new/Time_old`, higher is better):

| 500.perlbench_r | -0.19% |
| 502.gcc_r       | 0.26%  |
| 505.mcf_r       | 0.21%  |
| 520.omnetpp_r   | 0.73%  |
| 523.xalancbmk_r | 0.29%  |
| 525.x264_r      | 0.00%  |
| 531.deepsjeng_r | 0.00%  |
| 541.leela_r     | 4.50%  |
| 557.xz_r        | -0.19% |
|





================
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.
----------------
SjoerdMeijer wrote:
> 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.
That and also the third parameter to `PointerMayBeCaptured` which limits the number of uses of the `alloca` to explore to no more than 20.


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

https://reviews.llvm.org/D107281