[PATCH] D99354: [SimpleLoopUnswitch] Port partially invariant unswitch from LoopUnswitch to SimpleLoopUnswitch

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

In D99354#2773344 <https://reviews.llvm.org/D99354#2773344>, @fhahn wrote:

> In D99354#2763024 <https://reviews.llvm.org/D99354#2763024>, @jaykang10 wrote:
>
>> The below bugs are fixed.
>> https://bugs.llvm.org/show_bug.cgi?id=50279
>> https://bugs.llvm.org/show_bug.cgi?id=50302
>>
>> Even if this patch does not add the processed loop to loop pass manager again, the later loop transformations create loop with the header, which has partially invariant instructions, and add it to loop pass manager again. The loop pass manager runs SimpleLoopUnswitchPass with the loop again and it sometimes causes endless unswitch with some CFGs.
>>
>> In order to avoid the endless unswitch, the new change removes the partially invariant instruction from header as below.
>>
>>   //    +--------------------+
>>   //    |     preheader      |
>>   //    |  %a = load %ptr    |
>>   //    +--------------------+
>>   //             |     /----------------\
>>   //    +--------v----v------+          |
>>   //    |      header        |---\      |
>>   //    | %c = phi %a, %b    |   |      |
>>   //    | %cond = cmp %c, .. |   |      |
>>   //    | br %cond           |   |      |
>>   //    +--------------------+   |      |
>>   //             |               |      |
>>   //    +--------v-----------+   |      |
>>   //    |  store %ptr        |   |      |
>>   //    +--------------------+   |      |
>>   //             |               |      |
>>   //    +--------v-----------<---/      |
>>   //    |       latch        >----------/
>>   //    |  %b = load %ptr    |
>>   //    +--------------------+
>
> Hm, this seems quite fragile, as another pass may decide to move the load to the header again.

um... I thought the load would not be moved to header later because it is loop invariant and we usually try to move it to outside loop.

> In the legacy pass manager, the loop is annotated with metadata to avoid partially unswithcing the same loop multiple times (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp#L849) Can we do the same here?

Yep, I have seen the metadata. I was just not sure whether people will accept it or not. Let me try to add it.



================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/endless-unswitch.ll:1
+; RUN: opt -O3 -S < %s | FileCheck %s
+
----------------
fhahn wrote:
> We should avoid adding tests using `-O3` here. Can we reproduce the issue by running `simple-loop-unswtich` twice? Also it would be good to clean up the test a bit.
um... in order to reproduce the endless unswitch, it needs other passes. Let me try to reduce the test.


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

https://reviews.llvm.org/D99354