[PATCH] D52827: [LICM] Make LICM able to hoist phis

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

In https://reviews.llvm.org/D52827#1306034, @mkazantsev wrote:

> This rehoisting stuff has been wary for me before, and now I'm completely hesitant about what is going on there. Why do we end up hoisting to some block that does not dominate its users? Do we have a strong reason of doing so rather than finding the correct hoisting destination?


Let's look at the @phi_conditional_use test in hoist-phi.ll as an example:

  define i64 @phi_conditional_use(i32 %f, i32* %g) {
  entry:
    %cmp1 = icmp eq i32 %f, 1
    %cmp2 = icmp eq i32 %f, 0
    br label %loop
  
  loop:
    br i1 %cmp1, label %if.end, label %if.then
  
  if.then:
    br label %if.end
  
  if.end:
    %phi = phi i64 [ 0, %loop ], [ 1, %if.then ]
    br i1 %cmp2, label %loop.backedge, label %if.then2
  
  if.then2:
    %d = getelementptr inbounds i32, i32* %g, i64 %phi
    store i32 1, i32* %d, align 4
    br label %loop.backedge
  
  loop.backedge:
    br label %loop
  }

When looking at %loop we note that the branch uses a loop invariant condition. Then when looking at %if.end we see that %phi has loop invariant operands, and is the merge point of the branch we noted, so we copy the control flow and hoist the phi:

  define i64 @phi_conditional_use(i32 %f, i32* %g) {
  entry:
    %cmp1 = icmp eq i32 %f, 1
    %cmp2 = icmp eq i32 %f, 0
    br i1 %cmp1, label %if.end.licm, label %if.then.licm
  
  if.then.licm:
    br label %if.end.licm
  
  if.end.licm:
    %phi = phi i64 [ 0, %entry ], [ 1, %if.then.licm ]
    br label %loop
  
  loop:
    br i1 %cmp1, label %if.end, label %if.then
  
  if.then:
    br label %if.end
  
  if.end:
    br i1 %cmp2, label %loop.backedge, label %if.then2
  
  if.then2:
    %d = getelementptr inbounds i32, i32* %g, i64 %phi
    store i32 1, i32* %d, align 4
    br label %loop.backedge
  
  loop.backedge:
    br label %loop
  }

We then note that the branch at the end if %if.end has a loop invariant condition. We then look at %if.then2 and see that %d has loop invariant operands, and that it's in a block that's the target of a noted branch, so we duplicate control flow and hoist:

  define i64 @phi_conditional_use(i32 %f, i32* %g) {
  entry:
    %cmp1 = icmp eq i32 %f, 1
    %cmp2 = icmp eq i32 %f, 0
    br i1 %cmp1, label %if.end.licm, label %if.then.licm
  
  if.then.licm:
    br label %if.end.licm
  
  if.end.licm:
    %phi = phi i64 [ 0, %entry ], [ 1, %if.then.licm ]
    br i1 %cmp2, label %loop.backedge.licm, label %if.then2.licm
  
  if.then2.licm:
    %d = getelementptr inbounds i32, i32* %g, i64 %phi
    br label %loop.backedge.licm
  
  loop.backedge.licm:
    br label %loop
  
  loop:
    br i1 %cmp1, label %if.end, label %if.then
  
  if.then:
    br label %if.end
  
  if.end:
    br i1 %cmp2, label %loop.backedge, label %if.then2
  
  if.then2:
    store i32 1, i32* %d, align 4
    br label %loop.backedge
  
  loop.backedge:
    br label %loop
  }

Now we look at the store in %if.then2, which has loop invariant operands but can't be hoisted as it's not safe to hoist the store.

We're now done hoisting things, but we have the problem that %d does not dominate the store in %if.then2. So we have to move it somewhere where it does dominate %if.then2, but we also have to make sure it's after %phi, as it uses that as an operand. So it has to go in %if.end.licm after %phi.

Some comments on all of this:

At the time that we hoist %d we don't know if its uses are also going to get hoisted. We _could_ do something like rewrite LICM to be a two-step process where it first makes note of which instructions are going to get hoisted, then hoists them using knowledge of if their uses are going to get hoisted to figure out where they get hoisted. But that would be a lot more disruption than the current patch.

We could change things to never hoist things into conditional blocks i.e. %d would be hoisted directly to %if.end.licm. There's two reasons I didn't do this:

- As explained in the comment at line 715, hoisting to conditional blocks could in the future be used as away to be able to hoist instructions that are unsafe to execute unconditionally, e.g. the store could actually be hoisted to %if.then2.licm as it would only get executed in the cases where the store in the loop would have been executed.
- It seems a little strange to just speculatively execute instructions without limit because you're hoisting them outside of the loop. It's what LICM currently does, but it's at odds with the current behaviour elsewhere e.g. in SimplifyCFG which does the same thing (except hoisting instructions to their immediate predecessor block instead of outside a loop) but has a lot of (inconsistent) heuristics for deciding if it's a good idea or not. Doing it like this means we can leave it up to later passes to figure out if the instructions should be unconditional.
- Additionally: doing this would means that "the hoisted version of a block X for the purpose of control flow and phis" and "which block to hoist instructions from block X into" could be different blocks whereas they're currently always the same block, which would need some extra code to handle (though of course at the same time the code to do rehoisting would get removed so it could turn out to be about the same complexity-wise).


Repository:
  rL LLVM

https://reviews.llvm.org/D52827