Page MenuHomePhabricator

[PowerPC] Do not convert loop to HW loop if the body contains calls to lrint/lround
ClosedPublic

Authored by nemanjai on Oct 10 2019, 3:35 PM.

Details

Summary

These two intrinsics are lowered to calls so should prevent the formation of CTR loops.

This patch rather aggressively disables CTR loops if there are calls to unknown intrinsics so that we don't keep hitting similar issues in the future. I will certainly collect some data about the number of CTR loops we emit before and after the patch before committing such an "aggressively pessimistic" patch. Posting this early just to gauge what others think about this conservative behaviour.

Diff Detail

Event Timeline

nemanjai created this revision.Oct 10 2019, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 3:35 PM
nemanjai marked an inline comment as done.Oct 10 2019, 3:36 PM
nemanjai added inline comments.
lib/Target/PowerPC/PPCTargetTransformInfo.cpp
281 ↗(On Diff #224493)

@hfinkel What do you think about this conservative approach to prevent similar issues in the future? This is I think a third time I address a similar bug.

hfinkel added inline comments.Oct 23 2019, 6:13 AM
lib/Target/PowerPC/PPCTargetTransformInfo.cpp
281 ↗(On Diff #224493)

I really thought that I had replied to this already, but apparently not. I'm okay with that, but I'd like to do it in a separate commit, and we need to audit the existing intrinsics and add them as necessary (both ones that have straightforward codegen and ones like llvm.assume which have no codegen).

joerg added a subscriber: joerg.Oct 27 2019, 2:05 PM

Can we please get this or the other fix in RSN? Mesa hits this.

nemanjai updated this revision to Diff 226682.Oct 28 2019, 9:24 AM

Remove the change to make this conservatively correct until we can have a look at existing intrinsics that could/should prevent HW loop generation.

@hfinkel @joerg @vddvss What do you guys think? Should we get this fix committed and I'll follow-up with a look at all the other intrinsics that we currently have?

hfinkel accepted this revision as: hfinkel.Oct 28 2019, 9:29 AM

LGTM

This revision is now accepted and ready to land.Oct 28 2019, 9:29 AM
This revision was automatically updated to reflect the committed changes.