[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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151482