[PATCH] D59506: [ValueTracking][InstSimplify] Support min/max selects in computeConstantRange()

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Nice, no performance concerns anymore, looks reasonable to me.

In general, i really like that this isn't being solved by yet another monstrous
pattern-matching (well, excluding the internals of `matchSelectPattern()`:)).

Is the test coverage sufficient?



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5686
+      break;
+    default:
+      break;
----------------
lebedev.ri wrote:
> nikic wrote:
> > lebedev.ri wrote:
> > > No `SPF_ABS` / `SPF_[N]ABS` ?
> > Those wouldn't match because I'm bailing out early if I don't have a Constant operand.
> > 
> > However, I just found that InstructionSimplify already has handling for ABS/NABS in simplifyICmpWithAbsNabs(). I could just move that in here, save the duplicate matchSelectPattern() call, and also remove the performance concern (because we were already calling matchSelectPattern(), not it's just in a more general place...)
> Sounds good (maybe new-pm will magically make it easier to cache such things?),
> but i'm not sure how that would look.
> How about you reverse the order of patches slightly, first do "just move that in here", 
> and base this patch ontop of that new patch?
Posted before refreshing page, nvm.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5656
 
+static void setLimitsForSelect(const SelectInst &SI, APInt &Lower,
+                               APInt &Upper) {
----------------
`Select` isn't descriptive enough.
I'd go with `SelectPattern`, as this is not about the `select` inst, but about some specific pattern flavors.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5689-5692
+      Upper = *C + 1;
+      break;
+    case SPF_UMAX:
+      Lower = *C;
----------------
Might be best not to hope that the other APInt is set to zero already?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59506/new/

https://reviews.llvm.org/D59506