Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Paths

Table of Contentst

PowerPC: Don't hoist float multiply + add to fused operation on SPE
ClosedPublic
Actions

Authored by jhibbits on Apr 6 2020, 8:12 AM.

Details

Reviewers

shchenz
nemanjai

Group Reviewers

Restricted Project

Summary

SPE doesn't have a fmadd instruction, so don't bother hoisting a
multiply and add sequence to this, as it'd become just a library call.
Hoisting happens too late for the CTR usability test to veto using the CTR
in a loop, and results in an assert "Invalid PPC CTR loop!".

Diff Detail

Event Timeline

jhibbits created this revision.Apr 6 2020, 8:12 AM

Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:12 AM

Herald added subscribers: llvm-commits, shchenz, kbarton and 2 others. · View Herald Transcript

Harbormaster completed remote builds in B51955: Diff 255351.Apr 6 2020, 9:45 AM

Herald added a subscriber: • wuzish. · View Herald TranscriptApr 6 2020, 9:45 AM

lkail added a reviewer: shchenz.Apr 6 2020, 3:20 PM

This happens too late for the CTR usability test to veto using the CTR in a loop, and results in an assert "Invalid PPC CTR loop!".

This means after not hoisting fmul makes a case have "Invalid PPC CTR loop!" assertion? This is a little surprised for me. Hoist or not hoist fmul should not impact CTR register.
Looking forward to your case.

In D77558#1966207, @shchenz wrote:

This happens too late for the CTR usability test to veto using the CTR in a loop, and results in an assert "Invalid PPC CTR loop!".

This means after not hoisting fmul makes a case have "Invalid PPC CTR loop!" assertion? This is a little surprised for me. Hoist or not hoist fmul should not impact CTR register.
Looking forward to your case.

Sorry, on a further reading of the summary I can see it can be a little confusing. *hoisting* fmul + fadd to fma results in a library call, because SPE doesn't have a fma instruction. Unfortunately, this transform is performed long after a loop is transformed to a CTR with bdnz loop, so it can't be caught at the loop transform time, and block the loop. In addition to triggering that assert, hoisting two instructions into a function call is quite a pessimization anyway :)

I'll update the summary to clarify this.

jhibbits edited the summary of this revision. (Show Details)Apr 7 2020, 7:06 AM

adalava added a subscriber: adalava.Jul 5 2022, 12:20 PM

Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 12:20 PM

I think this is the right thing to do regardless of whether it affects CTR loops or not. The FMA is never faster with SPE and that should be marked as such. But yes, we still need a test case.