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

Anna Thomas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 20 04:52:39 PDT 2023

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?

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list