[PATCH] D50167: RFC: [SCEV] Add explicit representations of umin/smin

[Keno Fischer via Phabricator via llvm-commits]

loladiro added a comment.

> I have a general concern against such changes. In fact, you are introducing an alternative way to express the same thing. `umin(a, b)` and `umax(~a, ~b)` are the same, but now have 2 possible notations. It means that whatever pass or analysis that needs to recognize this pattern needs to be aware of both. Whenever the new node was not supported, it might be missed optimization opportunities. And we cannot know for sure how many such places there are, or will be. What motivation do you have for making this change? Is it strong enough to take a risk of missing optimization opportunities that I've just pointed out?

I can understand this concern. On the other hand, by obscuring the true meaning of the pattern (it's not trivial to detect that the argument is actually the negation of another argument - there's some code that tries to do it, but it only works for simple expression), so you also lose the opportunity to optimize based on the existence of `umin`. In general, I don't really see a good way to deal with non-integral address spaces in the absence of a general `umin`, short of just disabling these optimizations completely for such pointers, which is not desirable. I suppose another alternative would be to make a first class `Neg` node type (rather than expanding it through as subs and multiplies), such that (`~umax(~x, ~y)`) could be pattern matched back into the proper form in code generation. I'm not sure that's any better though, since it just pushed this problem further down the expression tree.

> Also, I didn't get the part about infinite recursion in the commit message. Is this the behavior you are observing in current SCEV, or it only happens with your patch? In former case, please submit a bug with a test on which you can see that. From my memory, we've fixed `getUMax` to avoid the inifinite recursion, and maybe the same fix is required for `SMax`.

Actually, upon doing some more testing here, I misdiagnosed the test failure (there was a heuristic pattern match to detect the old umin pattern - will push a simple fix monetarily). I'll also put up a separate revision to fix up the isKnownPredicate check for SMax to avoid having that be a behavior change in this revision.


Repository:
  rL LLVM

https://reviews.llvm.org/D50167