[PATCH] D70852: [InstCombine] Guard maxnum/minnum conversions with a TTI query

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D70852#1763946 <https://reviews.llvm.org/D70852#1763946>, @uweigand wrote:

> But in those cases like funnel shifts, common code (specifically SelectionDAGBuilder::visitIntrinsicCall) will handle the expansion if the target doesn't have an instruction.
>
> So if we do need to do that for minnum/maxnum, that would probably be the better place.
>
> However, this is still a bit different, because minnum/maxnum can also be the result of an explicit call to fmin/fmax in the source, and it would seem surprising to have that translated into a select sequence as well.  If we originally had an fmin/fmax call, which was translated into minnum/maxnum that we then couldn't further optimize, the current expansion (back to the fmin/fmax call) seems correct to me.


Handling this in SDAG sounds like the right solution to me. We don't want instcombine to rely on TTI because it's supposed to be canonicalizing target-independently.
It's worth noting that the transform that is being done in IR depends on FMF:

  // Canonicalize select of FP values where NaN and -0.0 are not valid as
  // minnum/maxnum intrinsics.

So I think SDAG can also detect the difference between IEEE-compliant source code that uses libm calls vs. loose/fast source code that got converted to the LLVM intrinsic. Ie, we can expand using fcmp+select vs. libcall based on the FMF.


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

https://reviews.llvm.org/D70852