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

[Török Edwin]

On 2009-06-17 22:55, Nick Lewycky wrote:
> 2009/6/17 Török Edwin <edwintorok at gmail.com <mailto:edwintorok at gmail.com>>
>
>     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.
>
>
> Unless I'm mistaken, we deliberately push them the other way so that
> we can eliminate ext(constant) inside of the umax. Also, since umax
> can take multiple operands we get to canonicalize "umax(zext(umax(a,
> b)), c)" into "umax(zext(a), zext(b), c)".
>
> There's an ordering defined in ScalarEvolutionExpressions.h
> (scConstant to scUnknown) which should match the inside-to-outside
> order that we want SCEVs to have when canonicalizing.

Ok, this is the wrong approach then, since I could possibly infloop if
SCEV prefers the original ordering.

>  
>
>     zext is very often encountered in SCEV expressions on x86-64, since
>     pointer indexes for GEP are i64.
>
>     Thoughts?
>
>
> This is a real problem that deserves a fix, I'm just not sure I like
> the one proposed. How hard would it be to add logic to detect this
> case to the ScalarEvolutionExpander? Just look inside the umax and see
> whether all its operands contains the same zext/sext/trunc and hoist
> it then.

Yes, that would be possible. However I would prefer a utility function
is ScalarEvolution that given a SCEV* transforms it into a SCEV* with
minimal operations.
ScalarEvolutionExpander could use that, and so could other clients of
ScalarEvolution.

As initial implementation it could transform OP1(OP2(a1), OP2(a2), ...,
OP2(aN)) -> OP2(OP1(a1, ..., aN)).
Besides umax for OP1, and zext/sext for OP2 is there any other situation
where this transform would be valid?

Best regards,
--Edwin