[PATCH] D103816: [SimpleLoopUnswich] Fix a bug on ComputeUnswitchedCost with partial unswitch

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2875
             continue;
-        } else if (!PartialIVInfo.InstToDuplicate.empty()) {
+        } else if (PartialIVCondBranch == &TI) {
           if (PartialIVInfo.KnownValue->isOneValue() &&
----------------
sanwou01 wrote:
> jaykang10 wrote:
> > sanwou01 wrote:
> > > Isn't this always true for a partial unswitch? If there is a partial unswitch candidate, PartialIVCondBranch will be the terminator of the loop header. When ComputeUnswitchedCost is called for a partial unswitch TI is *also* the terminator of the loop header. Did I miss something?
> > > 
> > > FWIW I also think that the previous condition is always true: for a partial unswitch, there is always at least one instruction in InstToDuplicate. (again, the terminator of the loop header!)
> > > Isn't this always true for a partial unswitch? If there is a partial unswitch candidate, PartialIVCondBranch will be the terminator of the loop header. When ComputeUnswitchedCost is called for a partial unswitch TI is *also* the terminator of the loop header. Did I miss something?
> > 
> > It is correct. The candidate of PartialIVCondBranch is the terminator of the loop header. However, there could be multiple unswitch candidates which are fully or partially loop invariant. In this case, we need to check whether the candidate is partially invariant or not. You can see the example on
> >  `partial_unswitch_exiting_block_with_multiple_unswitch_candidates` test function.
> > 
> > > FWIW I also think that the previous condition is always true: for a partial unswitch, there is always at least one instruction in InstToDuplicate. (again, the terminator of the loop header!)
> > 
> > As we can see on `hasPartialIVCondition`, we add instructions such as CMP, LOAD or GEP into InstToDuplicate for partially invariant cases. For fully invariant cases, logical OR, logical AND, True, False or loop invariant conditions are added. 
> > 
> > As we can see on below comment, we need to check the cost of duplicated successors. The duplicated successors are different according to the fully or partially invariant instructions.
> > ```
> >       // If this is a partial unswitch candidate, then it must be a conditional
> >       // branch with a condition of either `or`, `and`, their corresponding
> >       // select forms or partially invariant instructions. In that case, one of
> >       // the successors is necessarily duplicated, so don't even try to remove
> >       // its cost.
> > ```
> With `!FullUnswitch`, we know that we are considering a partial unswitch candidate. Could there be multiple *partial* unswitch candidates? Currently, it looks like there can only be a single partial unswitch candidate per loop, but maybe that's not necessarily true?
Um... you are right. `!FullUnswitch` means we assume logical OR, logical AND and partially invariant instructions.
Let me remove the `if (PartialIVCondBranch == &TI)`. 


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

https://reviews.llvm.org/D103816