Page MenuHomePhabricator

Don't crash when we see unallocatable registers in clobbers

Authored by george.burgess.iv on Oct 17 2017, 6:09 PM.



This is part 1 of addressing the feedback from D38479. Apologies in advanced if these miss anything obvious; I've spent very little time working with backend code. :)

This also fixes the immediate issue raised by the test-case for, but the moment you try to use FP/vector instructions with the integrated assembler, you'll get complaints. Since the spirit of -mgeneral-regs-only is "the assembler can use non-general registers," we'll probably need to address that, as well.

Diff Detail


Event Timeline

george.burgess.iv edited the summary of this revision. (Show Details)Oct 17 2017, 6:09 PM

Simplify test-case

(Sorry for the noise; I was trying to swap to uploading this diff first, but rather than hitting the "swap tab" shortcut to migrate the title/summary/etc. over, I hit cmd+enter. Doing that apparently submits the diff for review.)

efriedma added inline comments.Oct 20 2017, 12:57 PM
97 ↗(On Diff #119416)

Might as well just allow all ISD::Register nodes? The type of an ISD::Register isn't really significant anyway.

george.burgess.iv marked an inline comment as done.

Addressed all feedback. Thanks!

efriedma accepted this revision.Oct 23 2017, 12:39 PM

LGTM, with one minor adjustment.

948 ↗(On Diff #119908)

Somehow you lost a comment from the original code. Also, the if statement around the assertion is a little weird. Maybe something like the following?

assert((Regs[I] != SP ||
        DAG.getMachineFunction().getFrameInfo().hasOpaqueSPAdjustment()) &&
       "If we clobbered the stack pointer, MFI should know about it.")
This revision is now accepted and ready to land.Oct 23 2017, 12:39 PM
george.burgess.iv marked an inline comment as done.Oct 23 2017, 1:46 PM

Thanks again!

This revision was automatically updated to reflect the committed changes.