[PATCH] D129636: Fix a LSR debug invariance issue

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added a comment.

In D129636#3696260 <https://reviews.llvm.org/D129636#3696260>, @markus wrote:

> In D129636#3696240 <https://reviews.llvm.org/D129636#3696240>, @chrisjackson wrote:
>
>> I'm looking at the forgetValue() implementation. I think that as long as the SCEV for the value is preserved and accessable in SalvageDVI() then all is well in terms of the salvaging method.
>
> Right, I think that forgetting a SCEV does not invalidate the SCEV itself it is just that if we were to ask for it again then we would get a new object (that would still be equivalent though not the same object pointer wise).
>
>> In D129636#3648003 <https://reviews.llvm.org/D129636#3648003>, @jmorse wrote:
>>
>>> Thanks for hunting this down, caching layers affecting codegen sounds hard to debug.
>>>
>>> I'm no SCEV expert, so a couple of possibly obvious questions:
>>>
>>> - Is it safe to store the SCEV* for a Value that's then forgotten?
>>> - Is there a risk that the presence of debug-info will now cause a SCEV to be forgotten that _should_ have been remembered, because of unconditionally forgetting a SCEV if it's used by debug-info?
>>
>> I think the second point is the crux here. Perhaps there is a way to test if the SCEV already exists, then call forget conditionally.
>
> A few comments up I wrote which I think does what you are requesting
>
>> For the second point maybe `ScalarEvolution::getExistingSCEV()` could be used to see if this SCEV existed before analyzing the dbg.value or not so that we don't end up forgetting about values that we would not forget about hadn't it been for the dbg.value.
>
> If that makes sense I will update the patch.

Apologies, I overlooked your earlier response to that point. Your solution sounds good to me.


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

https://reviews.llvm.org/D129636