[PATCH] D129636: Fix a LSR debug invariance issue

[Chris Jackson via Phabricator via llvm-commits]

chrisjackson added a comment.

In D129636#3699269 <https://reviews.llvm.org/D129636#3699269>, @fhahn wrote:

> In D129636#3698967 <https://reviews.llvm.org/D129636#3698967>, @chrisjackson wrote:
>
>> In D129636#3696750 <https://reviews.llvm.org/D129636#3696750>, @fhahn wrote:
>>
>>> I think I am missing some context here, but this looks a bit suspicious. I am not sure if exposing and checking whether a SCEV exists is the right way to go.
>>>
>>> IIUC the main issue seems to be that we try to create SCEVs for all dbg intrinsics? Would it be possible to instead only get SCEV expressions for values we really need (that/s the induction variable I assume, for which there already should be SCEV expression)?
>>
>> It is necessary to obtain SCEVs for all the values used in dbg.value within the loop, as LSR may optimise out these values and these are salvaged by generating a DWARF expression from the SCEV expression. For further explanation see:
>> D105207 <https://reviews.llvm.org/D105207> and the EuroLLVM2022 talk on the method here :SCEV-Based debuginfo salvaging in Loop Strength Reduction <https://llvm.org/devmtg/2022-05/>.
>
> Right, but wouldn't it be sufficient to collect all expressions LSR optimizes? For those, it will already have created SCEV expressions for. This might be more difficult,  but I am wondering if it would be feasible at least in theory.

Currently the information for the dbg.value within the loop is cached before LSR executes, with DbgGatherSalvagableDVI(). If the implementation was changed to cache information on only the values that LSR optimises out, then the cacheing code would have to be moved inside the core LSR pass, I believe. I think this would require a significant change in the scev-salvaging implementation and also modifying the LSRInstance interface and implementation. I'm not keen on that idea, but I can have a further think and see if there is a better way.


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

https://reviews.llvm.org/D129636