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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:151
+  SelectPatternFlavor SPF = matchSelectPattern(Inst, A, B).Flavor;
+  // TODO: We should also detect abs and FP min/max.
+  if (SPF == SPF_SMIN || SPF == SPF_SMAX ||
----------------
hfinkel wrote:
> spatel wrote:
> > hfinkel wrote:
> > > Is there anything to do to address this TODO except for removing this check, the corresponding check below, and, to be good, adding a few test cases?
> > > 
> > > I don't want this artificially restricted, but I'm fine enabling the others in a follow-up commit (so that we can revert separately if necessary). This LGTM, but please do take care of the other cases in a follow-up commit, or add a comment explaining why that's non-trivial.
> > For abs, it should just be fixing the check and adding tests. I need to look closer at which FP variants are commutable. But yes, I'll do both of those after this patch.
> >  I need to look closer at which FP variants are commutable. But yes, I'll do both of those after this patch.
> 
> Ah, I should have mentioned. I looked through the code before I made the suggestion. I think that you only need exclude SPF_UNKNOWN. It looks like all of the others commute because matchSelectPattern specifically excludes matching floating-point forms that don't commute (it specifically excludes cases where signed-zeros could cause problems, NaNs might cause problems, etc.).
> 
Great - I was coming to that conclusion too now that I'm reading the code. Just need to come up with N test cases (and probably a pile of negative tests too). :)

I think we will need to handle the 'num' intrinsics separately (but similarly):

```
define double @maxnum_intrinsic(double %x, double %y) {
  %m1 = call double @llvm.maxnum.f64(double %x, double %y)
  %m2 = call double @llvm.maxnum.f64(double %y, double %x)
  %r = fadd double %m1, %m2
  ret double %r
}

```


Repository:
  rL LLVM

https://reviews.llvm.org/D41136