[PATCH] D151482: [LV] Add support for minimum/maximum intrinsics

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:936
+  case RecurKind::FMinimum:
+    return CmpInst::FCMP_OLT;
+  case RecurKind::FMaximum:
----------------
dmgreen wrote:
> anna wrote:
> > dmgreen wrote:
> > > If this is unused, could it be removed? I think it goes via getMinMaxReductionIntrinsicOp?
> > > 
> > > I'm not sure if the expansion to cmp+select would be valid without nnan and nsz fmf flags, creating the intrinsic like you have it sounds much better.
> > There are two options:
> > 1. Remove it and add a comment that usage directly goes through `getMinMaxReductionIntrinsicOp`.
> > 2. Change the predicate to the correct one: `FCMP_ULT`. Instead of ordered, we need unordered variants as seen by the langref:  https://llvm.org/docs/LangRef.html#id308 (it allows NaNs: QNaN or SNaN makes no difference I think). 
> > 
> > I prefer the second option for completeness (if for any reason we choose `getMinMaxReductionPredicate` API for some other purpose in future, not necessarily within `createMinMaxOp`).
> > 
> > What do you think?
> Yeah I was wondering about the same thing.
> minimum also handles -0.0 as less than 0.0, which means it isn't very obvious how to expand it out into fcmp+select. Something like this is needed: https://alive2.llvm.org/ce/z/iB2Dt7. (But that doesn't handle nan as Alive2 seems to think that SNaN turns into +inf, in a way I don't understand. So I wasn't able to test ULT vs OLT).
> 
> I think olt and ult will just pick differently between the first and second operand of the select if they are Nan though, not choose _which_ is a Nan. Ordered will chose the second operand if either operand is a nan. Unordered will pick the first. In neither case is that guaranteed to be the Nan.
> 
> So it would likely be valid to expand into fcmp+sel provoding it had nsz and nnan, but not without. It may be better to just remove it and rely on createMinMaxOp. We can hopefully remove getMinMaxReductionPredicate in the future if we get to the point where MinNum and MaxNum can be handled, with the necessary FMF.
agreed, thanks for checking it. I'll update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151482