This is an archive of the discontinued LLVM Phabricator instance.

Extend "idempotent" atomicrmw optimizations to floating point
ClosedPublic

Authored by reames on Feb 14 2019, 1:19 PM.

Details

Summary

Caution: I am assuming the values used on ConstantExpr::getBinOpIdentity are correct. If someone with floating point knowledge can confirm, that'd be great.

Extend the integer transform in the obvious way. I decided not to cast to integer type to be conservative for the moment. Happy to reverse that if desired.

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Feb 14 2019, 1:19 PM
jfb added a subscriber: scanon.Feb 14 2019, 1:22 PM
jfb added inline comments.
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
33 ↗(On Diff #186907)

@scanon for comment.

90 ↗(On Diff #186907)

I think you want to canonicalize to integer or with zero a well, and have casts. It's still just a load at the end of the day.

reames marked 2 inline comments as done.Feb 21 2019, 9:40 AM
reames added inline comments.
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
33 ↗(On Diff #186907)

@scanon ping on the FP question only

90 ↗(On Diff #186907)

Can do so, but do you mind if I do that in a separate change just for risk reduction?

jfb added inline comments.Feb 21 2019, 10:05 AM
lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp
90 ↗(On Diff #186907)

Totally fine by me.

Given no response from @scanon, and the constants appear elsewhere in LLVM fp transforms, I'd like to go ahead and land this. JF, how does that sound to you?

jfb accepted this revision.Feb 28 2019, 8:05 PM

Given no response from @scanon, and the constants appear elsewhere in LLVM fp transforms, I'd like to go ahead and land this. JF, how does that sound to you?

SGTM

This revision is now accepted and ready to land.Feb 28 2019, 8:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 10:02 AM