This is an archive of the discontinued LLVM Phabricator instance.

[CVP] Simplify non-overflowing saturating add/sub
ClosedPublic

Authored by nikic on May 30 2019, 2:04 PM.

Details

Summary

If we can determine that a saturating add/sub will not overflow based on range analysis, convert it into a simple binary operation. This is a sibling transform to the existing with.overflow handling.

Diff Detail

Event Timeline

nikic created this revision.May 30 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 2:04 PM
lebedev.ri added inline comments.May 30 2019, 2:12 PM
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
444

Why not add a new statistic
"Number of saturating arithmetics converted to normal arithmetics"
?

nikic updated this revision to Diff 202305.May 30 2019, 2:47 PM

Use separate statistic.

lebedev.ri accepted this revision.May 30 2019, 2:55 PM

Looks good

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
414–431

Hm, is this not preserving debugloc?

418

holding

This revision is now accepted and ready to land.May 30 2019, 2:55 PM
nikic marked an inline comment as done.May 31 2019, 9:06 AM
nikic added inline comments.
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
414–431

This does preserve debugloc. In this case it's handled by the IRBuilder API. (The overflows.ll test also does a debugify check.)

This revision was automatically updated to reflect the committed changes.
nikic reopened this revision.May 31 2019, 1:13 PM

Reverted in rL362254 due to assertion failure in clang tests.

This revision is now accepted and ready to land.May 31 2019, 1:13 PM
nikic updated this revision to Diff 202473.May 31 2019, 1:17 PM

Check for integer type before trying to simplify, as LVI does not support vectors of integers. Also fix this for the with.overflow case -- we've recently started accepting vector args for those as well, but it looks like nobody ever ran the vector version through -O1 :)

lebedev.ri marked an inline comment as done.May 31 2019, 1:22 PM

(can you please commit @llvm.uadd.with.overflow.v2i32 + fix; the @llvm.uadd.sat.v2i8 test; and the patch as three separate commits?)
Hmm, right, not sure how this should work for vectors, especially non-splat ones. Treat each line separately?

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
414–431

Great, thanks for checking!

418

err, right, folding (:

llvm/test/Transforms/CorrelatedValuePropagation/overflows.ll
831

Hmm, great catch!
I guess saturating vectors are rare since nobody hit this yet?

This revision was automatically updated to reflect the committed changes.
nikic marked an inline comment as done.May 31 2019, 1:51 PM
nikic added inline comments.
llvm/test/Transforms/CorrelatedValuePropagation/overflows.ll
831

Assuming you meant vector overflow here, then yes: This is only supported since recently -- more or less a side effect of having to deal with vector addo/subo in SDAG for saturating math expansion, so might as well allow in IR. But looks like nobody has actually used this yet, otherwise they would have definitely run into this issue :)