[PATCH] D108114: [LoopPeel] Peel if it turns invariant loads dereferenceable.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D108114#2950373 <https://reviews.llvm.org/D108114#2950373>, @fhahn wrote:

> In D108114#2949657 <https://reviews.llvm.org/D108114#2949657>, @reames wrote:
>
>> Before reviewing the patch, a high level question.  Fair warning, I am a bit worried about cost heuristic changes here, they tend to be delicate.
>>
>> Why do we need to peel this?  LICM is generally good at using speculation to hoist, and trivial unswitching is good at versioning the conditions to remove them.  I'd expect to see the motivating case already handled by some combination of existing transforms.  (You might have to iterate them a few times.  So is this working around a pass ordering issue?)
>
> Do you have any pointers at the kind of speculation LICM could be doing? I could not spot anything that would be applicable after an initial look.

LICM does two forms of legality reasoning for trap-safety.  Option 1 is guaranteed to execute.  Option 2 is proving that the hoisted load can't fault at the new location.  Looking at your test case (peel-multiple-unreachable-exits-for-vectorization.ll), option 1 does work (when iterated as you note).  Option 2 would require deref facts on the arguments A and B of @sum_2_at_with_int_conversion.  I don't see any reason why clang shouldn't be able to emit the deref info on those args?  (Assuming this is lowering a c++ reference to a std::vector<> at least.)  If it can, LICM should be able to speculate the loads for start and end from each vector outside the loop.

> I think the motivating case could indeed be handled by a set of existing passes, which was my first try. I think it would look roughly like the following:
>
> 1. run LICM to hoist out the invariant loads feeding the branch in the header,
> 2. run indvars to turn the check fed by the hoisted loads into an invariant check,
> 3. unswitch
> 4. LICM to hoist the now unconditional loads feeding the second branch in the unswitched loop,
> 5. run indvars to turn the second branch condition into an invariant.
> 6. unswitch
>
> (there's a small caveat that this pass order does not catch the specific test case in `peel-multiple-unreachable-exits-for-vectorization.ll`, but the unreduced motivating std::vector::at example, which I just noticed)
>
> The problem with that particular order is that it clashes with the existing order which is `LICM,Unswitch` in one loop pass manager, followed by `Indvars` and others in a separate loop pass manager. The passes are split up in different loop pass managers because we run function passes in between them (InstCombine & SimplifyCFG). Adjusting the pipeline seems like it would be quite a big shakeup and peeling off the first iteration seemed like a less invasive change and less work for the optimizations overall. Note that we would have to run `LICM,indvars,unswitch` once for each `std::vector::at` call/runtime check.
>
> What do you think? I think eliminating the separation between the 2 loop pass managers would be beneficial in its own right, but SimplifyCFG and InstCombine seems like a substantial gap to bridge.

I agree with your reasoning all the way through.

We'd previously explored the idea of adding loop versions of InstCombine and SimplifyCFG.  (See LoopSimplifyCFG, I don't remember where we stood on instcombine.)

One mid-term idea would be to keep the split loop pass, but have the second one contain the same passes as the first.  (Essentially, we'd have one unrolled iteration of the loop iteration with the instcombine and simplifycfg in place.)  Though, actually, I don't remember, do we even iterate loop passes to fixed point if there are changes being made to the loop?  (All of my context on this involves an out of tree pass order which solves this with brute force repetition.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108114