This is an archive of the discontinued LLVM Phabricator instance.

PowerPC: Treat llvm.fma.f* intrinsic as using CTR with SPE
ClosedPublic

Authored by jhibbits on Apr 22 2020, 2:00 PM.

Details

Reviewers
shchenz
Group Reviewers
Restricted Project
Commits
rG0138cc012506: PowerPC: Treat llvm.fma.f* intrinsic as using CTR with SPE
Summary

The SPE doesn't have a 'fma' instruction, so the intrinsic becomes a
libcall. It really should become an expansion to two instructions, but
for some reason the compiler doesn't think that's as optimal as a
branch. Since this lowering is done after CTR is allocated for loops,
tell the optimizer that CTR may be used in this case. This prevents a
"Invalid PPC CTR loop!" assertion in the case that a fma() function call
is used in a C/C++ file, and clang converts it into an intrinsic.

Diff Detail

Event Timeline

jhibbits created this revision.Apr 22 2020, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 2:00 PM
shchenz added inline comments.Apr 29 2020, 12:15 AM
llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
324

In PPCTargetLowering::PPCTargetLowering, We already have

if (Subtarget.hasSPE()) {
   setOperationAction(ISD::FMA  , MVT::f64, Expand);
   setOperationAction(ISD::FMA  , MVT::f32, Expand);
}

And there is a hook function TLI->isOperationLegalOrCustom(Opcode, EVTy) at line 415. I think better to use that function to check if one operation with a type is legal or not, similar with following Intrinsic::sqrt?

llvm/test/CodeGen/PowerPC/spe.ll
1357

Accidentally delete?

jhibbits marked 3 inline comments as done.Apr 30 2020, 9:45 AM
jhibbits added inline comments.
llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
324

I'm surprised I missed that, quite the oversight. Yes, that test below is much preferred, I was afraid I might need to play whack-a-mole on this, and wondering why that wasn't the case except for FMA. Fixing it to the better way.

llvm/test/CodeGen/PowerPC/spe.ll
1357

Yeah. a botched rebase sorting my local commits.

jhibbits updated this revision to Diff 261410.Apr 30 2020, 6:02 PM
jhibbits marked an inline comment as done.

Address feedback from @shchenz. Much better!

Questionable on the sorting where I put the intrinsic check, there doesn't
appear to be any kind of sort order, so hope it's fine.

shchenz accepted this revision as: shchenz.May 10 2020, 5:52 PM

LGTM

This revision is now accepted and ready to land.May 10 2020, 5:52 PM
This revision was automatically updated to reflect the committed changes.