Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.CodeGen/AVR::call.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AVR/call.ll -march=avr -mattr=avr6 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AVR/call.ll
20 msx64 debian > LLVM.CodeGen/AVR::dynalloca.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AVR/dynalloca.ll -march=avr | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AVR/dynalloca.ll
40 msx64 debian > LLVM.CodeGen/AVR::varargs.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mattr=sram,movw,addsubiw < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AVR/varargs.ll -march=avr | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AVR/varargs.ll
70 msx64 debian > Polly.ScopInfo::user_provided_assumptions.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo -polly-codegen-verify -pass-remarks-analysis="polly-scops" -polly-scops -disable-output < /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/user_provided_assumptions.ll 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/ScopInfo/user_provided_assumptions.ll
70 msx64 windows > LLVM.CodeGen/AVR::call.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\llc.exe < C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\CodeGen\AVR\call.ll -march=avr -mattr=avr6 | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\CodeGen\AVR\call.ll
View Full Test Results (7 Failed)

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
1–2

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

14–17

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
1–2

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.Thu, Jul 22, 7:33 PM
wt added a subscriber: wt.Sat, Jul 31, 11:26 PM
wt added a comment.Sun, Aug 1, 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.