[PATCH] D47037: [ValueTracking][EarlyCSE][InstCombine] Improve EarlyCSE of some absolute value cases.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:941-942
     // MIN(MIN(a, b), a) -> MIN(a, b)
-    if (SPF1 == SPF2)
+    if (SelectPatternResult::isMinOrMax(SPF1) && SPF1 == SPF2)
       return replaceInstUsesWith(Outer, Inner);
 
----------------
craig.topper wrote:
> spatel wrote:
> > Can you check that in as a preliminary cleanup? I think this patch should be just about EarlyCSE.
> > 
> > Ideally, we'd clean this up more - put the min/max predicate over the subsequent 3 transforms too and simplify that code (can FP min/max get in here?). 
> > 
> > Even better would be to move these folds to instsimplify. We're not creating new values here.
> I don't see any reason FP min/max can't get here.
> 
> What simplification does the min/max check provide for the other 3 transforms?
I might've been seeing something that isn't there. I was imagining if this block was pulled out as its own function, then you'd early exit if (!isMinOrMax), and that allows rearranging the subsequent checks into some shared form.

Do you know if the corresponding FP folds are done elsewhere, or are they missing, or does something prevent those?


https://reviews.llvm.org/D47037