[PATCH] D41136: [EarlyCSE] recognize commuted and swapped variants of min/max as equivalent (PR35642)

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D41136#955455, @spatel wrote:

> On closer inspection, I think matchSelectPattern is either not sufficient or broken because it doesn't handle -0.0 as we would require here:
>
>   declare void @self_destruct_if_neg_zero(double)  
>   define double @fmin_any_ordered_commute(double %x, double %y) {
>     %cmp1 = fcmp nnan olt double %x, %y                            ; if x=+0.0 and y=-0.0, returns false
>     %cmp2 = fcmp nnan olt double %y, %x                            ; if x=+0.0 and y=-0.0, returns false
>     %neg_zero_if_false = select i1 %cmp1, double %x, double %y     ; returns -0.0
>     %pos_zero_if_false = select i1 %cmp2, double %y, double %x     ; returns +0.0
>     call void @self_destruct_if_neg_zero(double %pos_zero_if_false)
>     ret double %neg_zero_if_false
>   }
>
>
> If we use what is returned by matchSelectPattern ( {SPF_FMINNUM, SPNB_RETURNS_ANY, false} ), we get:
>
> $ ./opt -early-cse fminmax.ll -S
>
>   define double @fmin_any_ordered_commute(double %x, double %y) {
>     %cmp1 = fcmp nnan olt double %x, %y
>     %cmp2 = fcmp nnan olt double %y, %x
>     %neg_zero_if_false = select i1 %cmp1, double %x, double %y
>     call void @self_destruct_if_neg_zero(double %neg_zero_if_false)  ; boom!
>     ret double %neg_zero_if_false
>   }
>  
>


This seems inconsistent with the intent of matchSelectPattern, at least based on the comment:

  // If the predicate is an "or-equal"  (FP) predicate, then signed zeroes may
  // return inconsistent results between implementations.
  //   (0.0 <= -0.0) ? 0.0 : -0.0 // Returns 0.0
  //   minNum(0.0, -0.0)          // May return -0.0 or 0.0 (IEEE 754-2008 5.3.1)
  // Therefore we behave conservatively and only proceed if at least one of the
  // operands is known to not be zero, or if we don't care about signed zeroes.
  switch (Pred) {
  default: break;
  case CmpInst::FCMP_OGE: case CmpInst::FCMP_OLE:
  case CmpInst::FCMP_UGE: case CmpInst::FCMP_ULE:
    if (!FMF.noSignedZeros() && !isKnownNonZero(CmpLHS) &&
        !isKnownNonZero(CmpRHS))
      return {SPF_UNKNOWN, SPNB_NA, false};
  }

It seems like there are cases missing here (e.g., this does not apply only to the GE/LE predicates)? Based on your example, it seems like we have the same problem with the GT/LT predicates.


Repository:
  rL LLVM

https://reviews.llvm.org/D41136