[PATCH] D33186: [InstCombine] Canonicalize clamp of float types to minmax in fast mode.

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1329
+          (CmpLHS != LHS && CmpLHS != RHS) ||
+          (CmpRHS != LHS && CmpRHS != RHS)) {
         CmpInst::Predicate Pred = getCmpPredicateForMinMax(SPF, SPR.Ordered);
----------------
efriedma wrote:
> This change isn't guarded for floating-point operations in particular... does this have an impact on integer clamp?  If not, why?
Yes, I missed that. This does have impact on integer clamp but I think it won't matter for end-to-end compilation because before this change integer cases were already handled during ISel. Anyway, there are two options here:
- Limit the transformation for floating-point operations only, like [[ https://reviews.llvm.org/differential/diff/104108/ | this ]]
- Leave the code as is and modify title/summary + add tests to document the change in the behavior for integers.

I would prefer to go with the first approach and do the change for integers in a separate review, if necessary. Unless someone has a preference for the second approach, of course.


https://reviews.llvm.org/D33186