Page MenuHomePhabricator

[AVR] Always expand STDSPQRr & STDWSPQRr
ClosedPublic

Authored by Patryk27 on Apr 11 2022, 11:13 AM.

Details

Summary

Currently, STDSPQRr and STDWSPQRr are expanded only during
AVRFrameLowering - this means that if any of those instructions happen
to appear _outside_ of the typical FrameSetup / FrameDestroy
context, they wouldn't get substituted, eventually leading to a crash:

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

This commit fixes this issue by moving expansion of those two opcodes
into AVRExpandPseudo.

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

Patryk27 created this revision.Apr 11 2022, 11:13 AM
Patryk27 requested review of this revision.Apr 11 2022, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 11:13 AM

cc @benshi001 🙂

So far I've just moved appropriate parts of code from the previous merge request (https://reviews.llvm.org/D114611), but I haven't yet applied 3.2 & 3.3 from https://reviews.llvm.org/D114611#3394757, since I don't quite understand those suggestions - currently the expansion uses either the Y _or_ the X register, and it looks like you'd rather for that instruction to always rely on X?

benshi001 added inline comments.May 3 2022, 11:50 PM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1221

Could you please add cases to show the use of R29R28 or R31R30 in different situations ?

llvm/test/CodeGen/AVR/stdwstk.ll
3

I perfer the .mir tests in your previous https://reviews.llvm.org/D114611.

It would be better to seperate to STDWSPQRr.mir and STDSPQRr.mir.

benshi001 added inline comments.May 4 2022, 12:15 AM
llvm/lib/Target/AVR/AVRFrameLowering.cpp
369

I do not think the removal of fixStackStores at here is good, since we seperate the preparation of R31R30 and the substitution of STDWSPQRr-> STDWPtrQRr. It makes the code hard to understand, and I also concern there might be other issues introduced.

So I suggest we keep this fixStackStores call.

This comment was removed by benshi001.
benshi001 added inline comments.May 4 2022, 5:49 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1217

"SP is expected as base pointer"

1221

Sorry, ignore this comment.

llvm/lib/Target/AVR/AVRFrameLowering.cpp
369

I suggest a moderate way:

Keep the second fixStackStores (with R31R30) and remove the first one (R29R28). And in the expand<AVR::STDWSPQRr>, add an extra line

assert(STI.getFrameLowering()->hasReservedCallFrame(MF) &&
       "unexpected STDWSPQRr pseudo instruction");
llvm/test/CodeGen/AVR/stdwstk.ll
3

Sorry, igmore this comment.

8–10

Remove these lines

; See:
; - https://reviews.llvm.org/D114611
; - https://reviews.llvm.org/D95664
benshi001 added inline comments.May 4 2022, 5:50 AM
llvm/lib/Target/AVR/AVRFrameLowering.cpp
369

Then we only substitute SP with R29R28.

Patryk27 updated this revision to Diff 427047.May 4 2022, 9:54 AM

Applying code review comments

Patryk27 marked 9 inline comments as done.May 4 2022, 9:59 AM

Thanks for the review - I've applied all the changes :-)

llvm/lib/Target/AVR/AVRFrameLowering.cpp
369

Sure, that's a nice approach!

llvm/test/CodeGen/AVR/stdwstk.ll
3

I'd love to add those somehow, but since expanding those opcodes reads hasReservedCallFrame(), we'd have to make those two *.mir tests execute AVRFrameAnalyzer, which seems clumsy to implement & use.

Patryk27 marked an inline comment as done.May 4 2022, 10:01 AM
benshi001 accepted this revision.May 4 2022, 7:40 PM
This revision is now accepted and ready to land.May 4 2022, 7:40 PM
This revision was automatically updated to reflect the committed changes.