This is an archive of the discontinued LLVM Phabricator instance.

[ppc64] fix bug in prologue that mfocrf's cr operand should be explict state instead of implicit
ClosedPublic

Authored by cycheng on Apr 18 2016, 10:05 PM.

Details

Reviewers
tjablin
kbarton
Summary

Diff Detail

Event Timeline

cycheng updated this revision to Diff 54157.Apr 18 2016, 10:05 PM
cycheng retitled this revision from to [ppc64] fix bug in prologue that mfocrf's cr operand should be explict state instead of implicit.
cycheng updated this object.
cycheng added reviewers: kbarton, tjablin.
cycheng added subscribers: hfinkel, nemanjai, llvm-commits.
kbarton added inline comments.Apr 25 2016, 12:35 PM
lib/Target/PowerPC/PPCFrameLowering.cpp
846–847

Why does this need to be put into a variable? I only see it used once below - can you not just put RegState::ImplicitKill directly in the MIB.addReg call below?

mgrang added a subscriber: mgrang.Apr 25 2016, 12:40 PM

Can you also add the testcase mentioned in the bug as a unit test?

Can you also add the testcase mentioned in the bug as a unit test?

Sure, I will add "-verify-machineinstrs" in test/CodeGen/PowerPC/crsave.ll, the option can catch this issue (I've tested)

lib/Target/PowerPC/PPCFrameLowering.cpp
846–847

Do you mean:

MIB.addReg(MustSaveCRs[i], MfcrOpcode == PPC::MFCR8 ? RegState::ImplicitKill : RegState::Kill);
kbarton accepted this revision.Apr 26 2016, 8:41 AM
kbarton edited edge metadata.

LGTM.

lib/Target/PowerPC/PPCFrameLowering.cpp
846–847

Sorry, for some reason I missed the initialization. This is fine as is.

This revision is now accepted and ready to land.Apr 26 2016, 8:41 AM
cycheng updated this revision to Diff 55156.Apr 26 2016, 7:33 PM
cycheng edited edge metadata.

Update testcase

cycheng closed this revision.Apr 26 2016, 8:05 PM

Committed r267660