[llvm-commits] [PATCH] Fix: instcombine does not get max if hidden by sext

[Tobias Grosser]

On 01/06/2011 05:49 AM, Frits van Bommel wrote:
> On Thu, Jan 6, 2011 at 7:49 AM, Chris Lattner<clattner at apple.com>  wrote:
>> On Jan 3, 2011, at 8:52 PM, Tobias Grosser wrote:
>>> I fixed a recent bug report, that blocked me on a large FORTRAN test case:
>>>
>>>     InstCombine:
>>>     X = sext x ; x<  c ? X : C-1
>>>     -->
>>>     X = sext x; X>  C-1 ? C-1 : X
>>>
>>>     Instead of calculating this with mixed types promote all to the
>>>     larger type. This enables scalar evolution to analyze this
>>>     expression. PR8866
>
> Why only for sext and not zext? Does scalar evolution understand those
> well enough that it's not needed there or was it just not useful for
> your particular test case?
> Or maybe the zext case is too different from the sext case to add easily?
I wanted to check first if the approach I took was OK. The new version
contains also zext support.

>> Generally, the logic involved is complex enough that it is probably best to pull it out into a static helper function.  This should make it easier to read.
>
> And since the "less than" and "greater than" cases seem to be quite
> similar, you could try to use a single helper function for both.
They are exactly the same except of the -1/+1 at the beginning. I moved this
difference into an if condition and use the same code for the rest.

> Is the fallthrough at the end of the "less than" case intentional? If
> so, a big '// FALLTHROUGH' would probably be helpful. If not, add a
> 'break', obviously. Either way, adding one at the end of the "greater
> than" case would future-proof it in case anyone adds extra cases.
No, it was not intentionally. Fixed.

Attached is the new version.

Thanks for your help
Tobi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-InstCombine-Match-min-max-hidden-by-sext-zext.patch
Type: text/x-diff
Size: 7630 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110107/2d0fa5ed/attachment.patch>