[PATCH] D82072: [InstCombine] Combine select & Phi by same condition

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

This patch looks like a straightforward extension of the existing code, but I don't have a good sense for control-flow corner cases, so please wait for another LGTM.

I think we should have at least one other test that has a switch...something like this:

  define i32 @select_phi_same_condition_switch(i1 %cond, i32 %x, i32 %y) {
  entry:
    br i1 %cond, label %if.true, label %if.false
  
  if.true:
    switch i32 %x, label %exit [
    i32 1, label %merge
    i32 2, label %merge
    ]
  
  exit:
    ret i32 0
  
  if.false:
    br label %merge
  
  merge:
    %phi = phi i32 [0, %if.true], [0, %if.true], [%y, %if.false]
    %s = select i1 %cond, i32 %x, i32 %phi
    ret i32 %s
  }



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2555
   // FIXME: Remove this workaround when freeze related patches are done.
   // For select with undef operand which feeds into an equality comparison,
   // don't simplify it so loop unswitch can know the equality comparison
----------------
mkazantsev wrote:
> aqjune wrote:
> > spatel wrote:
> > > xbolva00 wrote:
> > > > @spatel can you look at this fixme?
> > > That was added with D35811.
> > > 
> > > I don't know exact status, but I think there are still several steps to go before we can start adding freeze instructions to IR early in the optimization pipeline and not cause perf regressions.
> > > 
> > > cc @aqjune @nlopes
> > I'll be able to work on addressing performance regressions happening from freeze insertions from the middle of July.
> > There are two things that I have in my mind: (1) we need a function attribute saying that the argument has a frozen value (I also suggested it at https://reviews.llvm.org/D81678 ), (2) we need to update InstCombine patterns to work well on frozen arguments too.
> > For the former one, I'm seeing how the patch goes; if it it goes into a different direction, I'll suggest a patch by myself. For the latter one, my plan is to enable insertion of freeze from the later-most instcombine/simplifycfg pass first, and see which optimization needs to be updated.
> Hi, just to make sure: this discussion has nothing to do with the patch, right? From this thread, I could not figure out if some action is required from me regarding this patch or not.
Correct - these comments don't affect this patch AFAIK.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2472
 
   BasicBlockEdge TrueEdge(IDom, TrueSucc);
   BasicBlockEdge FalseEdge(IDom, FalseSucc);
----------------
It would help to add a code comment and/or example that describes what we are doing in this block.


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

https://reviews.llvm.org/D82072