This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Recognize min/max intrinsics
ClosedPublic

Authored by nikic on Sep 4 2020, 1:12 PM.

Details

Summary

Recognize umin/umax/smin/smax intrinsics and convert them to the already existing SCEV nodes of the same name.

In the future we'll want SCEVExpander to also produce the intrinsics, but we're not there yet...

Diff Detail

Unit TestsFailed

Event Timeline

nikic created this revision.Sep 4 2020, 1:12 PM
nikic requested review of this revision.Sep 4 2020, 1:12 PM
lebedev.ri accepted this revision.Sep 4 2020, 1:18 PM

This LG, thanks.

This revision is now accepted and ready to land.Sep 4 2020, 1:18 PM

Two afterthoughts:

  1. I suspect that until SCEV actually produces these intrinsics, this is undef-unsafe. Thought i suspect there are other similar problems, so i'm not sure if we should worry about it just yet.
  2. Should also handle abs:
----------------------------------------
define i32 @src(i32 %x, i1 %y) {
%0:
  %r = abs i32 %x, %y
  ret i32 %r
}
=>
define i32 @tgt(i32 %x, i1 %y) {
%0:
  %x_frozen = freeze i32 %x
  %t0 = sub i32 0, %x_frozen
  %r = smax i32 %x_frozen, %t0
  ret i32 %r
}
Transformation seems to be correct!
----------------------------------------
define i32 @src(i32 %x) {
%0:
  %r = abs i32 %x, 1
  ret i32 %r
}
=>
define i32 @tgt(i32 %x) {
%0:
  %x_frozen = freeze i32 %x
  %t0 = sub nsw i32 0, %x_frozen
  %r = smax i32 %x_frozen, %t0
  ret i32 %r
}
Transformation seems to be correct!
nikic added a comment.Sep 5 2020, 3:16 AM

Two afterthoughts:

  1. I suspect that until SCEV actually produces these intrinsics, this is undef-unsafe. Thought i suspect there are other similar problems, so i'm not sure if we should worry about it just yet.

Yes, that's right. This will get resolved once we start canonicalizing to intrinsics, and until then a lot of the min/max handling is undef-unsafe anyway.

  1. Should also handle abs:

Would you like to have that included in this patch or separately? I don't believe that SCEV currently recognizes abs in select form, so this one might need additional SCEVExpander support at the same time (to at least expand it into a reasonable select). There's also some other intrinsics that could be handled, e.g. usub.sat(a, b) is umax(a, b) - b.

Two afterthoughts:

  1. I suspect that until SCEV actually produces these intrinsics, this is undef-unsafe. Thought i suspect there are other similar problems, so i'm not sure if we should worry about it just yet.

Yes, that's right. This will get resolved once we start canonicalizing to intrinsics, and until then a lot of the min/max handling is undef-unsafe anyway.

  1. Should also handle abs:

Would you like to have that included in this patch or separately?

Separate patch is fine i think.

I don't believe that SCEV currently recognizes abs in select form,
so this one might need additional SCEVExpander support at the same time (to at least expand it into a reasonable select).

Maybe, but i don't anticipate such need.

There's also some other intrinsics that could be handled, e.g. usub.sat(a, b) is umax(a, b) - b.

This revision was landed with ongoing or failed builds.Sep 5 2020, 7:31 AM
This revision was automatically updated to reflect the committed changes.