Page MenuHomePhabricator

[PowerPC] Fix VSX clobbers of CSR registers
ClosedPublic

Authored by nemanjai on Oct 7 2019, 7:28 AM.

Details

Reviewers
hfinkel
lei
jsji
Group Reviewers
Restricted Project
Commits
rG7fbaa8097ecc: [PowerPC] Fix VSX clobbers of CSR registers
Summary

If an inline asm statement clobbers a VSX register that overlaps with a callee-saved Altivec register or FPR, we will not record the clobber and will therefore violate the ABI. This is clearly a bug so this patch fixes it.

Diff Detail

Event Timeline

nemanjai created this revision.Oct 7 2019, 7:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 7:28 AM
amyk added a subscriber: amyk.Oct 11 2019, 10:44 PM
amyk added inline comments.
test/CodeGen/PowerPC/inline-asm-vsx-clobbers.ll
2 ↗(On Diff #223596)

Should we add -verify-machineinstrs to the test case?

nemanjai marked an inline comment as done.Oct 28 2019, 6:07 PM
nemanjai added inline comments.
test/CodeGen/PowerPC/inline-asm-vsx-clobbers.ll
2 ↗(On Diff #223596)

Yup. We should. I'll add it on the next revision.

lei accepted this revision.Nov 12 2019, 7:18 AM
lei added a subscriber: lei.

LGTM.
Please address Amy's comment on commit.

This revision is now accepted and ready to land.Nov 12 2019, 7:18 AM
jsji accepted this revision.Nov 12 2019, 2:06 PM

LGTM as well.

lib/Target/PowerPC/PPCISelLowering.cpp
14320 ↗(On Diff #223596)

Do we need to consider 32 or 64 bits regclass here? If not, maybe we should add some comments to explain?

test/CodeGen/PowerPC/inline-asm-vsx-clobbers.ll
3 ↗(On Diff #223596)

Why we need -enable-ppc-quad-precision here?

nemanjai marked 2 inline comments as done.Nov 15 2019, 1:13 PM

Thanks for your comments. I'll address them and commit this.

lib/Target/PowerPC/PPCISelLowering.cpp
14320 ↗(On Diff #223596)

I believe you mean VSSRC/VSFRC (i.e. the scalar VSX registers). I believe those have to be named as f<N> or vs<N> in the clobbers list so we should still handle them correctly.

test/CodeGen/PowerPC/inline-asm-vsx-clobbers.ll
3 ↗(On Diff #223596)

Ha ha, good catch. That's just an errant option. I'll remove it on the commit.

This revision was automatically updated to reflect the committed changes.