[PATCH] D93734: [LoopDeletion] Insert an early exit from dead path in loop

Fri Jan 8 12:29:59 PST 2021

jonpa added a comment.

In D93734#2486760 <https://reviews.llvm.org/D93734#2486760>, @fhahn wrote:

> In D93734#2478272 <https://reviews.llvm.org/D93734#2478272>, @jonpa wrote:
>
>> I found out quickly that perhaps the hardest part of this was to update datastructures after changing the CFG in a loop... I have barely been able to build the benchmarks as it is, so I am in need of some good advice on how to update things after a loop change, for this patch to be usable. Currently I have changed things temporarily so that LI, DT, etc are recomputed after loop deletion (BTW, I found that only recomputing those analyses on master after LoopDeletion was not NFC, which surprised me... Is that a bug or expected with the aim to save compile time?)
>>
>> Statistics on SPEC-17 on SystemZ:
>
> Thanks for the update Jonas! It looks like the patch includes some required changes that still landed (D86844 <https://reviews.llvm.org/D86844>), which might impact the number of removed loops. It might be good to re-collect the statistics. I tried to collect stats with this patch on SPEC2000/SPEC2006/MultiSource for X86 with LTO, but unfortunately there have been a few crashes.

Sorry to hear about the crashes, hopefully it can all be fixed...  I rebased the patch and did a new run:

  master (patched to not preserve analyses after LoopDeletion for a fair comparison):

        7557                    loop-delete - Number of loops deleted

  current patch:
        2352                    loop-delete - Number of early exits inserted
        7420                    loop-delete - Number of loops deleted
           6                    loop-delete - Number of loops eliminated (no remaining blocks)
         439                    loop-delete - Number of skipped loops (SCEV 0)

>> @jdoerfert:
>>
>>> FWIW, I my goal is to make it something driven by a generic analysis, not another pattern we match.
>>
>> I agree with that, I have now made the patch more general.
>
> I think both this patch and  D93764 <https://reviews.llvm.org/D93764> require very similar analysis to find 'no-op' paths through loops (with the difference that  partial unswitching can allow stores to memory that does not clobber the condition). Do you think it would be worth unifying the analysis code?

To me it depends on what your goal is with the partial unswitching - do you mean to extend your patch to include multiple partially invariant conditions in the header region? (Or is that already a side-effect of revisiting the new loop? If so, there is no need to compute the "Top-region" like this patch does.). I guess we could perhaps have a common function that finds a unique exit block from SuccBB back to header and collect the memory references as well on that path...

Do you think partial unswitching will be able eventually to handle all the early-exit cases?  I haven't tried this patch yet on top of your patch, but that will be interesting :-)

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

https://reviews.llvm.org/D93734