[PATCH] D38566: [SimplifyCFG] don't sink common insts too soon (PR34603)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D38566#889031, @hfinkel wrote:

> > This is a minimal patch to solve PR34603:
> >  https://bugs.llvm.org/show_bug.cgi?id=34603
>
> Can you please summarize here what's going on and how you arrived at this solution? Last I recall, in PR34603, we were discussing select formation, and how it might not be desirable to form selects early. By "aggressive flattening" do you mean select formation? What you're doing here seems like more than just disabling select formation, but sinking? I also think that we might not want to sink early, at least some things (because we can't later prove it safe to re-hoist), but it's not immediately clear how all of these things are related.


Sorry for the invented vocabulary. 'Aggressive flattening' was meant to include the select formation, but that's not actually the root of the problem. The sinking is the root of the problem, so that's why I'm proposing to disable that in the early incarnation of SimplifyCFG. My intent (probably needing refinement) is to delay the transform that was added with https://reviews.llvm.org/rL279460 ( SinkThenElseCodeToEnd ) because that's where we regressed the examples in the bug reports. Let me illustrate using the example in the test cases (it's a slightly simplified form of the motivating example in the bug report):

1. When we start, we have redundant values across multiple blocks:

  entry:
  %xi_ptr = getelementptr double, double* %x, i64 %i
  %yi_ptr = getelementptr double, double* %y, i64 %i
  %xi = load double, double* %xi_ptr
  %yi = load double, double* %yi_ptr
  
  if:
  %xi_ptr_again = getelementptr double, double* %x, i64 %i
  %xi_again = load double, double* %xi_ptr_again
  
  else:
  %yi_ptr_again = getelementptr double, double* %y, i64 %i
  %yi_again = load double, double* %yi_ptr_again

early-cse can kill the redundant ops, so we just have the gep+load in 'entry' when it's done.

2. simplifycfg currently runs before early-cse in all -O# pipelines, and SinkThenElseCodeToEnd() applies this transform to create sunk ops in the 'end' block

  entry:
    %xi_ptr = getelementptr double, double* %x, i64 %i
    %yi_ptr = getelementptr double, double* %y, i64 %i
    %xi = load double, double* %xi_ptr, align 8
    %yi = load double, double* %yi_ptr, align 8
    %cmp = fcmp ogt double %xi, %yi
    br i1 %cmp, label %if, label %end
  
  if:                                          
    br label %end
  
  end:                                           
    %y.sink = phi double* [ %x, %if ], [ %y, %entry ]    <--- sunk phi of gep base pointers
    %new_gep = getelementptr double, double* %y.sink, i64 %i  <--- redundant geps got killed
    %new_load = load double, double* %new_gep, align 8  <--- redundant loads got killed
    ret double %new_load

3. At this point, there are no CSE opportunities. The initial geps/loads are not the same as the 'common' sunk ops. SimplifyCFG carries on and turns the 2-case phi into a select:

  %xi_ptr = getelementptr double, double* %x, i64 %i
  %yi_ptr = getelementptr double, double* %y, i64 %i
  %xi = load double, double* %xi_ptr
  %yi = load double, double* %yi_ptr
  %cmp = fcmp ogt double %xi, %yi
  %y.sink = select i1 %cmp, double* %x, double* %y
  %new_gep = getelementptr double, double* %y.sink, i64 %i
  %new_load = load double, double* %new_gep
  ret double %new_load



4. Based on https://bugs.llvm.org/show_bug.cgi?id=34603#c19 , I think we agreed that this is practically impossible to recover from in later passes. Therefore, I'm proposing to delay the initial sinking that creates 'common' ops out of potentially redundant ops. It's possible that we also, independently, want to delay select formation in other cases. It's also possible that I've used a sledgehammer where a chisel would've worked. :)


https://reviews.llvm.org/D38566