[PATCH] D113442: [InstCombine] Enable fold select into operand for FAdd, FMul, FSub and FDiv.

[Huihui Zhang via Phabricator via llvm-commits]

huihuiz added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:257
   case Instruction::Sub:   // Can only fold on the amount subtracted.
+  case Instruction::FSub:
   case Instruction::Shl:   // Can only fold on the shift amount.
----------------
spatel wrote:
> huihuiz wrote:
> > spatel wrote:
> > > Why exclude fdiv?
> > I checked with alive2, looks like adding fdiv, sdiv and udiv will trigger undefined behavior
> > https://alive2.llvm.org/ce/z/KvFYev
> >  
> I think the integer ops are not safe because we can trigger immediate UB that might have been avoided in the original code, but fdiv should be fine - it doesn't have any different UB characteristics vs. fadd/fmul/fsub in the default FP environment.
> https://alive2.llvm.org/ce/z/Yj_NA2
> (This is timing out on the online version when allowing undef/poison, so please double-check on a local machine.)
Thanks Sanjay for the explanation. Include 'fdiv' to allow fold on the divisor amount.

I did alive-tv run on my local machine with time out increased to 
./alive-tv src.ll tgt.ll -smt-to 100000000

But alive did not converge after 1.5 hour. Doing an overnight run now, will update result tomorrow morning.



================
Comment at: llvm/test/Transforms/InstCombine/select-binop-cmp.ll:330
+  %B = fadd float %x, %z
+  %C = select i1 %A, float %B, float %z
+  ret float %C
----------------
spatel wrote:
> spatel wrote:
> > Why is the compare constant (0.0 or -0.0) relevant for this fold?
> > 
> > The true/false operands should be swapped so we have coverage for the pattern that replaces the true value with a constant. Similarly for `fmul`, there should be two tests.
> A better question might be - why is there an fcmp in any of these tests? That isn't part of the minimal pattern is it?
Removing fcmp instruction. The minimal pattern to fold select into binop operand does not require a fcmp. I am moving this into a separate test. The original test select-binop-cmp.ll checks fcmp ignores the sign of 0.0 (for example test @select_fadd_fcmp), so keep the original test unchanged for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113442