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

Thu Nov 11 08:48:22 PST 2021

spatel added subscribers: aqjune, regehr, nlopes.
spatel 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.
----------------
huihuiz wrote:
> 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.
> 
Hmm...I think it's a good sign if it did not find a failure after 1.5 hours, but that is a long time to spend on 2 instructions. 
cc @nlopes @regehr @aqjune in case they see something wrong or room for improvement here:
https://alive2.llvm.org/ce/z/TtGDNR

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113442