This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Fix the eliminateCallFramePseudos method so that it always expands STDWSPQRr and STDSPQRr
AbandonedPublic

Authored by dylanmckay on Jan 28 2021, 10:34 PM.

Details

Reviewers
arsenm
bogner
Summary

Before this bug was fixed, the newly added testcase would fail with the message:

LLVM ERROR: Not supported instr: <MCInst XXX <MCOperand Reg:1> <MCOperand Imm:15> <MCOperand Reg:53>>

  where XXX is the OpCode of either the STDWSPQRr instruction or the STDSPQRr instruction.

The issue was that the ISel lowering pass would lower many stack slot stores to these
instructions, but the frame pointer elimination code (that is designed to rewrite these two
instructions to real instructions) was only designed to run for STDWSPQRr/STDSPQRr instructions
that appeared in the basic blocks that contained the FrameSetup/FrameDestroy instructions.

The bug was fixed by modifying the code so that it unconditionally runs on STDWSPQRr/STDSPQRr
instructions and always expands them with the relevant STDWPtrQRr or STDPtrQRr instructions.

The bug is fixed by making the TargetInstrInfo::{isFrameInstr(), getFrameSize()} methods
virtual, and overriding them in the AVR backend. The LLVM middle end now
treats STDWSPQRr/STDSPQRr instructions as zero-sized frame instructions
so that they get the same treatment and are rewritten during call frame
pseudo elimination process, which is the one time we have enough
information to correctly rewrite STDWSPQRr/STDSPQRr operations to the stack
pointer.

This bug was originally discovered due to the Rust compiler_builtins library. Its 0.1.37 release
contained a 128-bit software division/remainder routine that exercised this buggy branch in the code.

Diff Detail

Event Timeline

dylanmckay created this revision.Jan 28 2021, 10:34 PM
dylanmckay requested review of this revision.Jan 28 2021, 10:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 10:34 PM
Herald added a subscriber: wdng. · View Herald Transcript
dylanmckay planned changes to this revision.Feb 9 2021, 4:26 AM

I've discovered an issue, this is not yet ready for review.

dylanmckay updated this revision to Diff 322354.Feb 9 2021, 5:44 AM

Fix an assertion error caused by attempting to get the frame size of non-frame instructions

arsenm requested changes to this revision.Mar 30 2021, 3:45 PM

This seems like a confusion of what frame instructions means. What do these stores have to do with call frames? The stores should reference frame indexes representing fixed frame objects in the incoming arguments or local objects. The frame adjust instructions are for SP modifications in call sequences where instructions are directly referencing offsets off of the SP, which do not have corresponding frame indexes

llvm/test/CodeGen/AVR/bug-2021-01-29-complex-frame-pointer-usage.ll
2–3

Disassembling here is a little bit weird. Why isn't directly emitting asm sufficent?

15–18

This sentence doesn't make sense to me. Frame index elimination is unrelated to frame setup / frame destroy.

This revision now requires changes to proceed.Mar 30 2021, 3:45 PM

I just realized I've been reading "frame pointer elimination" as "frame index elimination" but this change still doesn't make sense to me. Why do you want to treat these stores as frame instructions when they don't modify the stack pointer?

Rahix added a subscriber: Rahix.Jun 19 2021, 4:26 AM
Rahix added inline comments.
llvm/test/CodeGen/AVR/bug-2021-01-29-complex-frame-pointer-usage.ll
2–3

Maybe I can shed some light on this: The underlying bug manifested itself in the LLVM ERROR: Not supported instr: error shown below. This happens because the generated assembly actually still contains the STDWSPQRr or STDSPQRr pseudo instructions and it only starts failing once it is attempted to encode those in machine code (obviously, as they do not exist). Thus, if the test just generated the assembly, it would pass erroneously.

@arsenm and/or @bogner (plus other reviewers)

Please tell / explain / elaborate / make clear that the flag "This revision now requires changes to proceed." is still valid after the "it only starts failing once it is attempted to encode those pseudo instructions in machine code" comment from a month ago.

Other information for the "AVR instruction set world" what more to do as being patience, is also appreciated.

xgupta added a subscriber: xgupta.Jul 22 2021, 7:33 PM
wt added a subscriber: wt.Jul 31 2021, 11:26 PM
wt added a comment.Aug 1 2021, 8:22 PM

I'm curious what needs to be done to push this forward. I would be happy to help if there's something I can do.

At the very least, I can confirm that the bug is real. And this issue it making it impossible to use a current toolchain to build software for AVR microcontrollers. If there's more that I can do, I'm willing to learn.

enaut added a subscriber: enaut.Aug 24 2021, 11:33 PM

I just realized I've been reading "frame pointer elimination" as "frame index elimination" but this change still doesn't make sense to me. Why do you want to treat these stores as frame instructions when they don't modify the stack pointer?

Stepping in to try to answer this since I'm interested in a fix here.

It's true that these stack-store pseudos do not modify the stack pointer. However, their expansion depends on which register is used as the stack pointer (Y or Z), and AVRFrameLowering::eliminateCallFramePseudoInstr() is currently the place where that is known.

PEI::replaceFrameIndices calls eliminateCallFramePseudoInstr() only for instructions for which isFrameInstr() returns true. When AVRFrameLowering::eliminateCallFramePseudoInstr() is called, fixStackStores is called in turn to expand any stack-store pseudos in the remainder of the basic block. So making isFrameInstr() return true for stack-store pseudos causes eliminateCallFramePseudoInstr() to be called for them and fixStackStores() to expand them. (At that point, the loop inside fixStackStores() becomes redundant, I think.)

Perhaps the work of expanding these pseudos should be done elsewhere? AVRExpandPseudoInsts seems like the obvious place, but I'm not sure whether it has access to the requisite knowledge of which register contains the stack pointer.

xgupta removed a subscriber: xgupta.Sep 21 2021, 11:10 AM
mhjacobson added inline comments.Sep 21 2021, 11:13 AM
llvm/lib/Target/AVR/AVRInstrInfo.cpp
578

If fixStackStores() is now being called for each STDWSPQRr and STDSPQRr, does the loop inside fixStackStores() still make sense?

jonnor added a subscriber: jonnor.Oct 10 2021, 3:12 AM
dylanmckay abandoned this revision.Nov 29 2021, 3:26 AM

I'm going to close this in favor of https://reviews.llvm.org/D114611 . I agree with Matt's comments about there being a bit of funny business going on in this PR, the concept is being generalized in core unnecessarily and does not strictly make sense in the place I put it. I think the approach in this other patch (thanks @Patryk27) is much more sensible.

Thanks for the reviews everybody. Very well thought out.

Also thanks for chiming in @mhjacobson, that is exactly where I was coming from. Well articulated. The conclusion about moving it to AVRExpandPseudoInsts is a good one that I don't think I made it to when I last looked at this some time ago (too long ago - I know!).