Page MenuHomePhabricator

InstSimplify: If divisor element is undef simplify to undef
ClosedPublic

Authored by zvi on Jan 24 2018, 8:25 AM.

Details

Summary

If any vector divisor element is undef, we can arbitrarily choose it be
zero which would make the div/rem an undef value by definition.

Diff Detail

Event Timeline

zvi created this revision.Jan 24 2018, 8:25 AM
spatel accepted this revision.Jan 24 2018, 8:50 AM

LGTM. Just curious - do we have vector intrinsics or any passes that create vector integer division?

This revision is now accepted and ready to land.Jan 24 2018, 8:50 AM
zvi added a comment.Jan 24 2018, 9:18 AM

LGTM. Just curious - do we have vector intrinsics or any passes that create vector integer division?

There is ongoing work starting with D41944 to make divide more friendly to auto-vectorization (or other optimizations that may want to speculate division). Though this patch is not directly connected to it.

This revision was automatically updated to reflect the committed changes.
spatel added a subscriber: RKSimon.Feb 12 2018, 4:13 PM

After discussing this with @RKSimon , I think this patch was wrong. Undef in a lane is not the same as undefined behavior in a lane.

So we're not allowed to assume the undef value is '0' here. This will cause trouble when paired with InstCombiner::SimplifyDemandedVectorElts().

Example:

define i8 @sdiv_undef_elt_vec(<4 x i8> %x) {
  %div = sdiv <4 x i8> %x, <i8 1, i8 2, i8 3, i8 4>
  %ext0 = extractelement <4 x i8> %div, i32 0
  %ext1 = extractelement <4 x i8> %div, i32 1
  %add = add i8 %ext0, %ext1
  ret i8 %add
}

Simplification based on demanded lanes could replace the last 2 constant divisor elements with 'undef'. This patch will then have the whole function produce undef. That can't be right.

We have the correct logic in 'isUndefShift()':

// If all lanes of a vector shift are undefined the whole shift is.

udiv etc. have undefined behavior if any element of the divisor is undef. Even without optimizations, it could raise SIGFPE on x86. (See also D41944.)

Simplification based on demanded lanes could replace the last 2 constant divisor elements with 'undef'.

That shouldn't happen.

udiv etc. have undefined behavior if any element of the divisor is undef. Even without optimizations, it could raise SIGFPE on x86. (See also D41944.)

Simplification based on demanded lanes could replace the last 2 constant divisor elements with 'undef'.

That shouldn't happen.

Ah - thanks! This reminds me of the bug from D4424. So IIUC, it is ok for SimplifyDemandedVectorElts() to replace constants in ops that can't trap (are safe to speculate). But just like it's not ok to do that shuffle transform for div/rem, we can't replace constants in div/rem.