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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Hi Keno,

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'd also like to know @sanjoy 's opinion on that.

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`.


Repository:
  rL LLVM

https://reviews.llvm.org/D50167