HomePhabricator

[InstSimplify] fold integer min/max intrinsics with limit constant

Authored by spatel on Sun, Jul 26, 6:33 AM.

Description

[InstSimplify] fold integer min/max intrinsics with limit constant

Details

Committed
spatelSun, Jul 26, 6:41 AM
Parents
rG61ced4b87a80: GlobalISel: Handle 'n' inline asm constraint
Branches
Unknown
Tags
Unknown

Event Timeline

nikic added a subscriber: nikic.Sun, Jul 26, 7:26 AM

Thanks! Another good InstSimplify candidate would be umin(x, x) -> x.

Do we also want to do the general "umin(x, y) -> x if x <= y" etc fold in InstSimplify? I guess that would take care of both umin(x, x) and the case being implemented here, at the expensive of a more expensive analysis.

Thanks! Another good InstSimplify candidate would be umin(x, x) -> x.

Yes, I'll get that 1 in another commit. I can think of a few other patterns.
For example, we'll also want to do something like umin.i8(x, 255) -> x (but only if x is known not poison?)

Do we also want to do the general "umin(x, y) -> x if x <= y" etc fold in InstSimplify? I guess that would take care of both umin(x, x) and the case being implemented here, at the expensive of a more expensive analysis.

Yes, that's an open question. So we would call SimplifyICmpInst(ule/uge, x, y) from the umin case within simplifyBinaryIntrinsic() and get the full icmp analysis. That's definitely more powerful, but there's probably no way to know the cost until we start canonicalizing to these intrinsics in instcombine?

Yes, I'll get that 1 in another commit. I can think of a few other patterns.
For example, we'll also want to do something like umin.i8(x, 255) -> x (but only if x is known not poison?)

I don't think we want to give umin any poison blocking semantics. If one of the arguments is poison the result is poison, so this fold should be fine unconditionally. (Possibly it would be worth clarifying this in langref, but I believe this is the default semantic for intrinsics.)

Do we also want to do the general "umin(x, y) -> x if x <= y" etc fold in InstSimplify? I guess that would take care of both umin(x, x) and the case being implemented here, at the expensive of a more expensive analysis.

Yes, that's an open question. So we would call SimplifyICmpInst(ule/uge, x, y) from the umin case within simplifyBinaryIntrinsic() and get the full icmp analysis. That's definitely more powerful, but there's probably no way to know the cost until we start canonicalizing to these intrinsics in instcombine?

I think we should start out with having the transform, and then relaxing/moving it if it shows up as a problem in profiles. The current min/max representation with an icmp+select will already perform the full icmp folding. One difference to the status quo would be that we'd probably want to check both x >= y and x <= y to fold to x or y respectively.