[PATCH] D106058: [DAG] Fold select(cond,binop(x,y),binop(x,z)) -> binop(x,select(cond,y,z))

Thu Jul 15 11:12:14 PDT 2021

lebedev.ri added inline comments.

================
Comment at: llvm/test/CodeGen/NVPTX/fast-math.ll:150
 ; CHECK: selp.f32
+; CHECK: div.rn.f32
   %x = fdiv arcp float %a, %divisor
----------------
RKSimon wrote:
> tra wrote:
> > RKSimon wrote:
> > > lebedev.ri wrote:
> > > > tra wrote:
> > > > > tra wrote:
> > > > > > This looks like it may be a performance regression. Judging by the name of the tests, we do expect to see reciprocal and multiplies, not `div`.
> > > > > > 
> > > > > Never mind. These changes are not in the final version of the patch.
> > > > Then you were already missing some kind of an inverse transform,
> > > > because middle-end would have already done this fold:
> > > > https://godbolt.org/z/cb38zEj9Y
> > > I made a kuldge fix in rG3cc38703d5ab05be0b01c31f829d19b47f183c5f so it still tests the combineRepeatedFPDivisors code path which appeared to be their purpose. But as @lebedev.ri said, we're unlikely to see the original test code patterns from real world code as InstCombine will have already broken the pattern.
> > This version of the patch resulted in `div` showing up in the PTX. The godbolt example above looks good and still generates rcp/mul/mul, which is what we want.
> > 
> > If the generated PTX changes to use `div` once godbolt picks up these changes, that would be a regression. Changing the test just hides the problem. It might've been better to disable the test with a comment that the test is still correct, but that there may be an issue in NVPTX which needs to be fixed.
> > 
> > I'm not sure yet why/how your change affectes NVPTX. I'll check what's going on.
> > 
> > 
> This shows the llc output before/after opt -O3 https://godbolt.org/z/dz4PGvjEf
> The godbolt example above looks good and still generates rcp/mul/mul, which is what we want.

No it doesn't:
https://godbolt.org/z/P43Y5E76E

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106058