[llvm-commits] [PATCH] fold umax(zext A, zext B) -> zext (umax(A, B))

[Török Edwin]

On 2009-06-17 23:25, Dan Gohman wrote:
> On Jun 17, 2009, at 11:35 AM, Török Edwin wrote:
>
>   
>> Hi,
>>
>> I noticed that umax (zext t1 %X to %t0, zext t1 %Y to %t0) isn't  
>> folded,
>> the attached patch folds this into:
>> umax (zext t1 %X to %t0, zext t1 %Y to %t0).
>>
>> It also folds umax (sext t1 %X to %t0, sext t1 %Y to %t0) ->  sext t1
>> (umax (%X, %Y)) to %t0.
>>
>> zext is very often encountered in SCEV expressions on x86-64, since
>> pointer indexes for GEP are i64.
>>
>> Thoughts?
>>     
>
> Another question to ask is whether this kind of thing belongs in
> ScalarEvolution, or if it would be more appropriate for
> instcombine. Instcombine looks at all instructions in a program,
> while ScalarEvolution typically only looks at those related to
> loop iteration. Also, instcombine could more easily handle more
> generalized cases of this optimization, for example with umin.
>
> On the other hand, there are cases where it makes sense to do
> such simplifications in ScalarEvolution. Can you given an
> example where you're seeing this kind of code?
>   

It doesn't have much to do with code generation/optimization, but rather
with analysis.
I have a pass that tries to find buffer overflow bugs, doing that
involves lots of umax() expressions.
If I can move the zext out of umax I can decide quite early that an
access is valid:

int foo(unsigned n, unsigned i)
{
  char *a = malloc(n);
  if (!a)
   return -1;
  if (i < n)
    a[i]=0;
}

Compiling that on 32-bit, I can decide immediately. On 64-bit I need the
additional step of getting rid of zexts inside umax.
Of course a solver can decide either way (with zext or not), but for
such a simple case I prefer to solve the equation early myself.

Of course I could do this all in my pass, but I thought the SCEV code
would benefit from it generally.
Doing one zero ext, and one umax should be cheaper than 2 zero exts and
one umax. Also its easier to analyze expressions if you
don't have so many zero/sign extensions and truncations.


> One minor note on the patch itself:
>
>  > +      unsigned bits = Ty->getPrimitiveSizeInBits();
>  > +      if (!bits) {
> [...]
>  > +    if (maxBits) {
>
> Within ScalarEvolution code, you can use
> ScalarEvolution::getTypeSizeInBits, which has the advantage of
> TargetData information (when available). Using that instead
> of getPrimitiveSizeInBits would eliminate the need for the
> two if statements quoted here.
>   

Thanks, I'll use that in the next iteration of the patch (for sCEVExpander).

Best regards,
--Edwin