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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel 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 ||
----------------
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.).



Repository:
  rL LLVM

https://reviews.llvm.org/D41136