[AVR] Expand STDWSPQRr & STDSPQRr, approach #2

[Patryk27]

Summary

This takes a different approach to solve https://reviews.llvm.org/D95664: instead of hacking around isFrameInstr(), I've moved parts of the logic to AVRMachineFunctionInfo which allowed me to move the registry-checking stuff directly into the AVRExpandPseudo pass, simplifying rest of the code; I think it's the cleaner approach for the issue posted on that MR.

This is my first merge request to LLVM, so probably the quality is subpar, but I'm ofc. open to any comments; thanks! :-)

[Patryk27]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

2114

just saw there's MBBI.getParent()->getParent() (or something like that) that returns associated function, too

llvm/lib/Target/AVR/AVRMachineFunctionInfo.h

84 ↗

(On Diff #389870)

passing MachineFunction "back in here" feels like cheating, but I can't see another way at the moment

[Patryk27]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

2112

also, this ofc. needs to be refactored to use the EXPAND macro below

[dylanmckay]

Hey @Patryk27, very nice patch, I would not have guessed it was your first LLVM PR.

I've left a few style comments. I need to review it a bit deeper for correctness which I plan to do during the week sometime.

Also, I noticed this patch uses the same-ish base commit from master as https://reviews.llvm.org/D95664 (from 6 months or so ago), which means it doesn't actually cleanly apply on LLVM master and has a merge conflict in AVRFrameLowering.cpp and so it needs a rebase on LLVM main branch

Cheers for the patch @Patryk27

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

2113

Recommend you split the logic STI.getFrameLowering()->hasReservedCallFrame(MF) ? AVR::R29R28 : AVR::R31R30 into a method, with some reasonable name like `getStackStoreBaseRegister

2121

The Block type is actually MachineBasicBlock (I would link you to the source code, but GitHub is having an outage at the moment ....) suffice to say it the typedef at line 47 of this file

MI.getParent() is a machine basic block hidden by the typedef (always the parent of current element of MBBI)

So MI.getParent() == MBB

And MI.getParent()->getParent() == MBB.getParent()

The suggestion is to use MBB.getParent() rather than MBBI.getParent()->getPArent()

2204

Minor: Move this above next to STDWPtrQRr so similar operations are similarly ordered

llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.ll

1 ↗

(On Diff #390220)

This test is good but it would be good to also have a mir test as well (like the other pseudo instruction tests in the same folder) to make sure that the instructions STDSPQRr and STDWSPQRr are always covered. Technically, the current test could become a false positive in the future if the instruction selection layer changes and the test passes bypassing the instruction you meant to test. Pretty unlikely in this situation though. But it would still be good to have it just to make sure it is covered with an MIR test too.

6 ↗

(On Diff #390220)

The '6th sub-basic block' seems liable to change, I think it should be changed to a regex so that it doesn't break if LLVM common changes the generated basic block structure or order as does sometimes happen (see https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-regex-matching-syntax )

What might be a better idea: add an LLVM IR inline ASM statement with a NOP or something at the top of the function body. The generated ASM must have this inline ASM in its output, and it must also be after the function epilogue, therefore you could do CHECK: std Y+13, r18 and then CHECK-NEXT: std Y+14, r19 and then CHECK-NEXT: nop

But on the other hand, is the basic block CHECK actually useful? Perhaps we only need the two instruction checks.

Suggestion is: either make the basic block CHECK-LABEL use regex. or remove it. Either works

[dylanmckay]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

2113

I thought I deleted this comment as I changed my mind. Please ignore - there is no need to split it into a method

[Patryk27]

llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.ll

1 ↗

(On Diff #390220)

Technically, the current test could become a false positive in the future if the instruction selection layer changes and the test passes bypassing the instruction you meant to test.

True, true - how about I move this file to llvm/test/CodeGen/AVR/bug-$issuenumber.ll (not to lose the context of "this used to be an edge case"), and then create two separate MIR tests for those opcodes?

[dylanmckay]

llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.ll

1 ↗

(On Diff #390220)

Good idea, yup

[Patryk27]

Thanks for the review! Seizing the day, I've moved all of the regression tests to a common directory called regression - I think it's a bit tidier this way 🙂

[Patryk27]

llvm/lib/Target/AVR/AVRFrameLowering.cpp

320

Besides adding an early exit a few lines above, this logic was not changed - Phabricator seems to have a hard time figuring out whitespaces in the diff

llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.mir

17

Shouldn't those two also have implicit-dif $sp? It looks like the codegen drops it somewhere.

[couchand]

Thanks for your work on this @Patryk27.

I've tested the change set by cherry picking onto the Rust fork and compiling a suite of example programs. All of my verification has passed without error.

I will continue to try to come up with a test case that exercises the potential issue I raised inline, but please do let me know if I've missed something there. Thanks again!

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1221

Perhaps I'm missing something, but where is Z being initialized? I'm happy with the use of Y (I'm fine trusting the comment in the old code that Y is guaranteed to hold a copy of SP, though maybe I shouldn't just trust that).

The old code initialized Z immediately prior to converting the pseudo stores into real stores. I can't see the same guarantee being made here. Do we know that Z still holds the adjusted frame pointer value? At first glance it seems like intervening instructions could easily clobber it.

[Patryk27]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1221

That's a great question, and the answer is: I'm not sure 😅

On a parallel matter, I wonder why we're using the Z in the first place - couldn't we always refer to R29R28? Seems like that's what the other backends (e.g. RISC-V) do.

[dylanmckay]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1211

note to self (and others if they care)

considerations I haven't verified yet:

• AVRFrameLowering, the original location of this code, is earlier on in the codegen process than AVRExpandPseudoInsts. Before this patch, the allocation of Z and the replacements of the stack pointer with it were isolated to the "frame setup" pass(es) of LLVM. After this patch, the allocation of Z stays in the frame setup pass, but the substitutions of it are moved to a later AVRExpandPseudos pass. It's conceivable that other passes may clobber the valid ranges of that the Z instruction contains the frame pointer and inserting another usage of it later in the process will actually read an invalid valid. I haven't verified this yet

1221

It is safe to assume that the existing code is correctly initializing Y as the frame pointer, it does so here: https://github.com/llvm/llvm-project/blob/36b0325c442a3669c2eb2c6fcaeb2cb57445c851/llvm/lib/Target/AVR/AVRFrameLowering.cpp#L109-L111

The main complication around the frame pointer in the existing backend which is what I was worried about (not anymore), is some special hax in the somewhere in the backend that detects when the register allocator has allocated the Y register already but then subsequently figures out the function needs stack spills, which then forbids the previously valid allocation of the now-FP Y (the backend fixes this clash somehow). But this patch doesn't touch any of that so no worries there.

The old code initialized Z immediately prior to converting the pseudo stores into real stores. I can't see the same guarantee being made here. Do we know that Z still holds the adjusted frame pointer value? At first glance it seems like intervening instructions could easily clobber it.

Yes, I agree.

Obvious prerequisite context: at function entry, the stack pointer register (SP) is the frame pointer. The stack pointer never disappears.

If the function has no variable stack allocations (= a fixed stack frame size), then you don't actually need to have a frame pointer register to operate on the stack because you know the size of the stack (therefore the layout of the stack too) so you if you wanted to read/write to the frame within the function body, you could achieve this without a dedicated frame pointer register by directly reading/writing from SP into a temp reg, adjusting the pointer as needed as you know the stack layout, and executing the memory operation). If you were to write the assembly this way, you get the benefit of having 33.333..% extra pointer variables available to your code, because you never reserved Z(R28R29) for the frame pointer. The downside however is that every stack operation inside a function without a dedicated FP is now maybe 3-4 instructions rather than 1, etc.

The AVR backend does this in many cases. All functions have access to SP register no matter what, obvious. Some functions have access to a dedicated frame pointer register
The AVR backend will dedicate Y(R28R29) as the dedicated frame pointer, and initialize it in function prologue, if and only if a dedicated frame pointer is "needed". Currently, a dedicated frame pointer is needed if these conditions are satisfied.

So, if there are no spills, no allocas, and no arguments passed on the stack: then you won't have a dedicated frame pointer and if you wish to access the stack you'll need to manually store/save SP into a temp register of your choosing and do the operation that way. This is what the old code you're replacing was doing here. At the very start and very end of the block, Z(R31R30) gets manually assigned as a temporary "frame pointer" register, for the duration of the function epilogue basic block that is passed into fixStackStores at the end.

Conclusion:

So, the Z register can be considered a valid "temporary" frame pointer register and be used as such as you are doing. The key point though is that this substitution can only be done if: the function has no dedicated frame pointer (because only then will AVRFrameLowering.cpp store the stack pointer in Z register contain the frame pointer (that's still true after your patch) AND if you know for sure that the stack store/load instruction you're expanding originates from and only from the AVRFrameLowering.cpp if (Opcode == TII.getCallFrameSetupOpcode()) conditional that is actually setting Z.

Perhaps we can add some assertion to this function that ensures that you're only doing the Z substitution if you know the previous criteria I mentioned are valid (no dedicated FP + the instruction must have come from the if (Opcode == TII.getCallFrameSetupOpcode()) { conditional below. There might be a better way to do it tho, I'm still thinking

[dylanmckay]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1221

because you never reserved Z(R28R29)

typo, should be:

because you never reserved Y(R28R29)

llvm/test/CodeGen/AVR/regression/D114611.ll

2

I like the idea of moving the "bug coverage" tests to a subdir.

What do you reckon about renaming the directory past-bugs or something else rather than regression. The latter feels to generic - all of the tests, even outside the AVR directory, are there for regression so the name feels a bit meaningless.

[Patryk27]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1221

Hmm, I think that one thing still escapes me: in here - https://github.com/llvm/llvm-project/blob/36b0325c442a3669c2eb2c6fcaeb2cb57445c851/llvm/lib/Target/AVR/AVRFrameLowering.cpp#L359-L371 - why couldn't we just always operate on R29R28, forgetting about the Z register entirely? (assuming we expanded STDWSPQRr to always access Y, that is.)

I've been looking at the other backends and I can't for the life of me figure out why we need a second register whatsoever 🤔

llvm/test/CodeGen/AVR/regression/D114611.ll

2

I've named it regression after AMDGPU's one, but sure - past-bugs sounds fine, too :-)

[arsenm]

llvm/test/CodeGen/AVR/regression/D114611.ll

2

AMDGPU does not have regression subdirectory. Also I don't see any clear distinction between a regression test and any other

[Patryk27]

llvm/test/CodeGen/AVR/regression/D114611.ll

2

There's https://github.com/llvm/llvm-project/tree/60f5614931b43871791393df74689d3b2b05c329/llvm/test/MC/AMDGPU/regression.

I was under the impression that a "regression test" meant a "test for a past bug" (under which condition e.g. our or.ll wouldn't be technically a regression test, but a typical unit / integration one). But yeah, seems like this phrase has a more general meaning that I thought.

[Patryk27]

llvm/test/CodeGen/AVR/regression/D114611.ll

2

than I thought*

[Patryk27]

Edit: this is a wrong lead; please see: https://reviews.llvm.org/D114611#3201290.

---

So I think something this patch does is not right - I'm not sure how to prepare a minimized, llvm-only example, but if you try to run this code:

#![no_std]
#![no_main]
#![feature(bench_black_box)]

use atmega_hal::{pins, usart::Usart0, Peripherals};
use core::hint::black_box;
use panic_halt as _;
use ufmt::*;
use void::{ResultVoidExt, Void};

pub type Clock = atmega_hal::clock::MHz20;

#[atmega_hal::entry]
fn main() -> ! {
    let dp = Peripherals::take().unwrap();
    let pins = pins!(dp);

    let mut serial =
        Usart0::<Clock>::new(dp.USART0, pins.pd0, pins.pd1.into_output(), 115200.into());

    fun(&mut serial, 23, 23);
    fun(&mut serial, 24, 24);
    fun(&mut serial, 25, 25); // commenting-out this line makes the code work

    loop {}
}

#[inline(never)]
fn fun(serial: &mut dyn uWrite<Error = Void>, x: u32, y: u32) {
    let _ = black_box(serial);
    let _ = black_box(x / y);
}

[dependencies]
atmega-hal = { git = "https://github.com/Rahix/avr-hal", features = ["atmega328p", "rt"] }
embedded-hal = "0.2"
panic-halt = "0.2"
ufmt = "0.1"
void = { version = "1", default-features = false }

[profile.release]
panic = "abort"
codegen-units = 1
opt-level = 1
lto = false

... you'll see that the 25 / 25 division triggers what seems to be a CPU reset, and in reality is caused by a ret on a corrupted stack frame.

I've managed to nail the problem down to a miscompilation in __udivmodsi4 (https://github.com/rust-lang/rust/issues/82242#issuecomment-996805546), as if the ldd instructions before st / std were removed (?), but I'm not sure how to investigate it further 😅

[Patryk27]

I've got some progress!

https://pastebin.com/PUSLqL46 (that's division as compiled from https://github.com/rust-lang/compiler-builtins; sorry for pastebin, but I wanted to avoid spamming this thread with even more code)

llc-ing that code with -march=avr returns what seems to be valid code, but with -march=avr -mcpu=atmega328p the st / std look shady:

.LBB0_60:                               ; %_ZN17compiler_builtins3int19specialized_div_rem11u32_div_rem17h69a9140ee9b21fa0E.exit.i
        ldi     r24, 0
        cpi     r30, 0
        cpc     r31, r24
        breq    .LBB0_62
; %bb.61:                               ; %bb2.i
        st      Z, r12
        std     Z+1, r13
        std     Z+2, r6
        std     Z+3, r7

The stack-corruption flow goes: ldi -> cpi -> cpc -> breq (not taken) -> st (and here we refer to invalid Z (?)).

Edit 1:
On a second thought, that very part seems to be alright - it refers to:

if let Some(rem) = rem {
    *rem = quo_rem.1;
}

So the stack corruption must be caused by r30 & r31 containing trashy values, not the contents of the rem parameter, as the codegen expects. With -opt-bisect-limit=0 the output looks "more correct" (although I haven't executed it yet):

.LBB0_64:                               ; %_ZN17compiler_builtins3int19specialized_div_rem11u32_div_rem17h69a9140ee9b21fa0E.exit.i
	ldi	r24, 0
	cpi	r16, 0
	cpc	r17, r24
	breq	.LBB0_66
	rjmp	.LBB0_65
.LBB0_65:                               ; %bb2.i
	movw	r30, r16
	st	Z, r8
	std	Z+1, r9
	movw	r30, r16
	std	Z+2, r6
	std	Z+3, r7
	rjmp	.LBB0_66

Edit 2:
Seems like the peephole optimizer overwrites usages of r16:r17 with r30:r31, but it doesn't do it everywhere (?), causing some parts of the code to refer to r30:r31 thinking it contains rem, while in reality it doesn't.

Edit 3:
Stepping-through the code with GDB proves that the optimization is legal; what's curious though is that it looks like the rem argument gets scrambled even before the function is ran!

It looks like the numbers passed in here:

fun(&mut serial, 25, 25);

... get mistaken for the rem parameter! So it's as if performing 25 / 25 tried to do:

*((*uint16_t) 0x19) = something;

... which just happens to overwrite the stack; for instance 30 / 30 works, since it happens to refer to an unused memory region.

Edit 4:
Hmm, when expanding LLVM's div, shouldn't we pass 0 as the third argument, to zero-out rem? Seems like we're not doing that, which means that if r16:r17 happen to contain a non-zero value...

Edit 5:
I guess expansions for division & remainder were designed with avr-gcc's runtime in mind, right (https://github.com/gcc-mirror/gcc/blob/31048012db98f5ec9c2ba537bfd850374bdd771f/libgcc/config/avr/lib1funcs.S#L1633)? That'd explain the missing zeroing-out :-)

Edit 6:
🤦 rustc now ignores the no-compiler-rt field in target.json, which means that AVR builds are being linked with compiler-rt instead of the avr-gcc's runtime; well, what a journey - from a possible bug in the codegen straight to "nah, it's a linker issue" 😅

(pingback: https://github.com/rust-lang/rust/issues/82242#issuecomment-997400373)

[Patryk27]

I've just found a small bug with our current approach: if we generate STDWPtrQRr from within AVRExpandPseudo, then we don't have a chance to run AVRRelaxMem on that newly-created instruction (so, bottom line, functions with larger stacks trigger assert(Imm <= 62 && "Offset is out of range");).

Maybe we should merge AVRRelaxMem with AVRExpandPseudo or we should have a separate pass for STDWPtrQRr?

[Patryk27]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1261

I'm not sure if that's correct, but the previous approach:

buildMI(MBB, MBBI, AVR::POPWRd)
    .addDef(Ptr.getReg(), getKillRegState(Ptr.isKill()));

... wasn't correct either, since it's not possible to addDef() a killed register (this crashes LLVM, but there was no test to check it on our side).

[benshi001]

It would be better to write a clear commit message with clear title.

[AVR] Expand pseudo instructions STDWSPQRr & STDSPQRr

1. What is the issue / Why there is a failure in instruction selection ?
2. How you fix it.

This is just my suggestion, a unique clear commit message is necessary when committing to the main branch, nobody would like to track links to learn all about your work.

[benshi001]

I find some test files locations are moved, I suggested you create a seperate patch for that, and this patch only focuses on fixing the instruction selection failure.

Also I can help review and commit the other patch.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1246

I suggest we make another patch for merge AVRRelaxMem::relax<AVR::STDWPtrQRr> into expand<AVR::STDWPtrQRr>, for the case Imm >= 63. And we select that merge patch as baseline / parent of current patch.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1234

Why we need an extra Dst local variable here? I did not find there is any more use besides .getReg() .

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1240

Also, I did not find any other use of Src

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1253

buildMI(MBB, MBBI, AVR::SBCIWRdK)
        .addReg(DstReg, RegState::Define)
        .addReg(DstReg, RegState::Kill)
        .addImm(-Imm);

The original DstReg is indeed killed.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1256

buildMI(MBB, MBBI, AVR::STWPtrRr)
    .addReg(Dst.getReg(), RegState::Kill)
    .addReg(Src.getReg(), getKillRegState(SrcIsKill));

No matter DstIsKill is true or false, the new DstReg is always killed.

1261

I think

if (!DstIsKill)
   buildMI(MBB, MBBI, AVR::POPWRd).addReg(DstReg, RegState::Define);

[benshi001]

I summary my suggestion in general, excluding some inline comments about coding details.

1. Seperate the location movement of test files into a different patch A.
2. Seperate the elimination of AVRRelaxMemPass / combination into expand<AVR::STDWPtrQRr> to a different patch B.

3. I also concern current solution is a bit agresssive, so I would like to suggest a moderate way.

3.1 Kill the function fixStackStores and its orginal calls.
3.2 Mark the definition of STDWSPQRr/STDSPQRr with Defs = [R31R30] (Most ordinary STDWSPQRr/STDSPQRr will be substituted before regalloc, so that is fine.)
3.3 Still implement expand<AVR::STDSPQRr> and expand<AVR::STDWPQRr> as current patch does, but do not substitute to AVR::STDPtrQRr/AVR::STDWPtrQRr, we should do real instruction expansion with several buildMI calls. Since we have marked Defs = [R31R30] to STDWSPQRr/STDSPQRr, it is safe to expand to

in r30, 62
in r31, 63
subiw z, offset
st z, Rsrc

or

in r30, 62
in r31, 63
std z+offset, Rsrc

[Patryk27]

Thanks for the review!

For the time being, I have extracted the first set of changes into:
https://reviews.llvm.org/D122533

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1234

Right, I think it must have been an oversight on my side; I have fixed it in the follow-up merge request (https://reviews.llvm.org/D122533).

1246

Sure! https://reviews.llvm.org/D122533

[Patryk27]

Changes in this revision were split into separate revisions, so this one is not needed anymore.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

   return true;
 }
 
+template <>
+bool AVRExpandPseudo::expand<AVR::STDSPQRr>(Block &MBB, BlockIt MBBI) {
+  MachineInstr &MI = *MBBI;
+  const MachineFunction &MF = *MBB.getParent();
+  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
+
+  assert(MI.getOperand(0).getReg() == AVR::SP && "Unexpected register");
+
+  MI.setDesc(TII->get(AVR::STDPtrQRr));
+  MI.getOperand(0).setReg(STI.getFrameLowering()->hasReservedCallFrame(MF) ? AVR::R29R28 : AVR::R31R30);
+
+  return true;
+}
+
+template <>
+bool AVRExpandPseudo::expand<AVR::STDWSPQRr>(Block &MBB, BlockIt MBBI) {
+  MachineInstr &MI = *MBBI;
+  const MachineFunction &MF = *MBB.getParent();
+  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
+
+  assert(MI.getOperand(0).getReg() == AVR::SP && "Unexpected register");
+
+  MI.setDesc(TII->get(AVR::STDWPtrQRr));
+  MI.getOperand(0).setReg(STI.getFrameLowering()->hasReservedCallFrame(MF) ? AVR::R29R28 : AVR::R31R30);
+
+  return true;
+}
+
 template <>
 bool AVRExpandPseudo::expand<AVR::INWRdA>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;

     EXPAND(AVR::STWPtrPiRr);
     EXPAND(AVR::STWPtrPdRr);
     EXPAND(AVR::STDWPtrQRr);
+    EXPAND(AVR::STDSPQRr);
+    EXPAND(AVR::STDWSPQRr);
     EXPAND(AVR::INWRdA);
     EXPAND(AVR::OUTWARr);
     EXPAND(AVR::PUSHWRr);

llvm/lib/Target/AVR/AVRFrameLowering.cpp

   return true;
 }
 
-/// Replace pseudo store instructions that pass arguments through the stack with
-/// real instructions.
-static void fixStackStores(MachineBasicBlock &MBB,
-                           MachineBasicBlock::iterator MI,
-                           const TargetInstrInfo &TII, Register FP) {
-  // Iterate through the BB until we hit a call instruction or we reach the end.
-  for (MachineInstr &MI :
-       llvm::make_early_inc_range(llvm::make_range(MI, MBB.end()))) {
-    if (MI.isCall())
-      break;
-
-    unsigned Opcode = MI.getOpcode();
-
-    // Only care of pseudo store instructions where SP is the base pointer.
-    if (Opcode != AVR::STDSPQRr && Opcode != AVR::STDWSPQRr)
-      continue;
-
-    assert(MI.getOperand(0).getReg() == AVR::SP &&
-           "Invalid register, should be SP!");
-
-    // Replace this instruction with a regular store. Use Y as the base
-    // pointer since it is guaranteed to contain a copy of SP.
-    unsigned STOpc =
-        (Opcode == AVR::STDWSPQRr) ? AVR::STDWPtrQRr : AVR::STDPtrQRr;
-
-    MI.setDesc(TII.get(STOpc));
-    MI.getOperand(0).setReg(FP);
-  }
-}
-
 MachineBasicBlock::iterator AVRFrameLowering::eliminateCallFramePseudoInstr(
     MachineFunction &MF, MachineBasicBlock &MBB,
     MachineBasicBlock::iterator MI) const {
   const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
   const AVRInstrInfo &TII = *STI.getInstrInfo();
 
-  // There is nothing to insert when the call frame memory is allocated during
-  // function entry. Delete the call frame pseudo and replace all pseudo stores
-  // with real store instructions.
   if (hasReservedCallFrame(MF)) {
-    fixStackStores(MBB, MI, TII, AVR::R29R28);
     return MBB.erase(MI);
   }
 

   unsigned int Opcode = MI->getOpcode();
   int Amount = TII.getFrameSize(*MI);
 
-  // ADJCALLSTACKUP and ADJCALLSTACKDOWN are converted to adiw/subi
-  // instructions to read and write the stack pointer in I/O space.
-  if (Amount != 0) {
-    assert(getStackAlign() == Align(1) && "Unsupported stack alignment");
-
-    if (Opcode == TII.getCallFrameSetupOpcode()) {
-      // Update the stack pointer.
-      // In many cases this can be done far more efficiently by pushing the
-      // relevant values directly to the stack. However, doing that correctly
-      // (in the right order, possibly skipping some empty space for undef
-      // values, etc) is tricky and thus left to be optimized in the future.
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPREAD), AVR::R31R30).addReg(AVR::SP);
-
-      MachineInstr *New =
-          BuildMI(MBB, MI, DL, TII.get(AVR::SUBIWRdK), AVR::R31R30)
-              .addReg(AVR::R31R30, RegState::Kill)
-              .addImm(Amount);
-      New->getOperand(3).setIsDead();
-
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPWRITE), AVR::SP).addReg(AVR::R31R30);
-
-      // Make sure the remaining stack stores are converted to real store
-      // instructions.
-      fixStackStores(MBB, MI, TII, AVR::R31R30);
-    } else {
-      assert(Opcode == TII.getCallFrameDestroyOpcode());
-
-      // Note that small stack changes could be implemented more efficiently
-      // with a few pop instructions instead of the 8-9 instructions now
-      // required.
-
-      // Select the best opcode to adjust SP based on the offset size.
-      unsigned addOpcode;
-      if (isUInt<6>(Amount)) {
-        addOpcode = AVR::ADIWRdK;
-      } else {
-        addOpcode = AVR::SUBIWRdK;
-        Amount = -Amount;
-      }
+  if (Amount == 0) {
+    return MBB.erase(MI);
+  }
+
+  assert(getStackAlign() == Align(1) && "Unsupported stack alignment");
 
-      // Build the instruction sequence.
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPREAD), AVR::R31R30).addReg(AVR::SP);
+  if (Opcode == TII.getCallFrameSetupOpcode()) {
+    // Update the stack pointer.
+    // In many cases this can be done far more efficiently by pushing the
+    // relevant values directly to the stack. However, doing that correctly
+    // (in the right order, possibly skipping some empty space for undef
+    // values, etc) is tricky and thus left to be optimized in the future.
+    BuildMI(MBB, MI, DL, TII.get(AVR::SPREAD), AVR::R31R30).addReg(AVR::SP);
+
+    MachineInstr *New =
+        BuildMI(MBB, MI, DL, TII.get(AVR::SUBIWRdK), AVR::R31R30)
+            .addReg(AVR::R31R30, RegState::Kill)
+            .addImm(Amount);
+    New->getOperand(3).setIsDead();
+
+    BuildMI(MBB, MI, DL, TII.get(AVR::SPWRITE), AVR::SP).addReg(AVR::R31R30);
+  } else {
+    assert(Opcode == TII.getCallFrameDestroyOpcode());
 
-      MachineInstr *New = BuildMI(MBB, MI, DL, TII.get(addOpcode), AVR::R31R30)
-                              .addReg(AVR::R31R30, RegState::Kill)
-                              .addImm(Amount);
-      New->getOperand(3).setIsDead();
+    // Note that small stack changes could be implemented more efficiently
+    // with a few pop instructions instead of the 8-9 instructions now
+    // required.
 
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPWRITE), AVR::SP)
-          .addReg(AVR::R31R30, RegState::Kill);
+    // Select the best opcode to adjust SP based on the offset size.
+    unsigned addOpcode;
+    if (isUInt<6>(Amount)) {
+      addOpcode = AVR::ADIWRdK;
+    } else {
+      addOpcode = AVR::SUBIWRdK;
+      Amount = -Amount;
     }
+
+    // Build the instruction sequence.
+    BuildMI(MBB, MI, DL, TII.get(AVR::SPREAD), AVR::R31R30).addReg(AVR::SP);
+
+    MachineInstr *New = BuildMI(MBB, MI, DL, TII.get(addOpcode), AVR::R31R30)
+                            .addReg(AVR::R31R30, RegState::Kill)
+                            .addImm(Amount);
+    New->getOperand(3).setIsDead();
+
+    BuildMI(MBB, MI, DL, TII.get(AVR::SPWRITE), AVR::SP)
+        .addReg(AVR::R31R30, RegState::Kill);
   }
 
   return MBB.erase(MI);

     SavedRegs.set(AVR::R28);
   }
 }
+
 /// The frame analyzer pass.
 ///
 /// Scans the function for allocas and used arguments

 
   bool runOnMachineFunction(MachineFunction &MF) override {
     const MachineFrameInfo &MFI = MF.getFrameInfo();
-    AVRMachineFunctionInfo *FuncInfo = MF.getInfo<AVRMachineFunctionInfo>();
+    AVRMachineFunctionInfo *AFI = MF.getInfo<AVRMachineFunctionInfo>();
 
     // If there are no fixed frame indexes during this stage it means there
     // are allocas present in the function.

       for (unsigned i = 0, e = MFI.getObjectIndexEnd(); i != e; ++i) {
         // Variable sized objects have size 0.
         if (MFI.getObjectSize(i)) {
-          FuncInfo->setHasAllocas(true);
+          AFI->setHasAllocas(true);
           break;
         }
       }

           }
 
           if (MFI.isFixedObjectIndex(MO.getIndex())) {
-            FuncInfo->setHasStackArgs(true);
+            AFI->setHasStackArgs(true);
             return false;
           }
         }

 
 char AVRFrameAnalyzer::ID = 0;
 
-/// Creates instance of the frame analyzer pass.
 FunctionPass *createAVRFrameAnalyzerPass() { return new AVRFrameAnalyzer(); }
 
 /// Create the Dynalloca Stack Pointer Save/Restore pass.

 
 char AVRDynAllocaSR::ID = 0;
 
-/// createAVRDynAllocaSRPass - returns an instance of the dynalloca stack
-/// pointer save/restore pass.
 FunctionPass *createAVRDynAllocaSRPass() { return new AVRDynAllocaSR(); }
 
 } // end of namespace llvm

llvm/lib/Target/AVR/AVRRegisterInfo.cpp

   Reserved.set(AVR::SPH);
   Reserved.set(AVR::SP);
 
-  // We tenatively reserve the frame pointer register r29:r28 because the
+  // We tentatively reserve the frame pointer register r29:r28 because the
   // function may require one, but we cannot tell until register allocation
   // is complete, which can be too late.
   //

llvm/lib/Target/AVR/AVRSubtarget.h

   /// of an ELF object file.
   unsigned getELFArch() const {
     assert(ELFArch != 0 &&
-           "every device must have an associate ELF architecture");
+           "every device must have an associated ELF architecture");
     return ELFArch;
   }
 

llvm/test/CodeGen/AVR/pseudo/STDSPQRr.mir

+# RUN: llc -O0 -run-pass=avr-expand-pseudo %s -o - | FileCheck %s
+
+--- |
+  target triple = "avr--"
+  define void @test() {
+  entry:
+    ret void
+  }
+...
+
+---
+name:            test
+body: |
+  bb.0.entry:
+
+    ; CHECK: STDPtrQRr $r31r30, 10, $r1, implicit-def $sp
+
+    STDSPQRr $sp, 10, $r1, implicit-def $sp
+...

llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.mir

+# RUN: llc -O0 -run-pass=avr-expand-pseudo %s -o - | FileCheck %s
+
+--- |
+  target triple = "avr--"
+  define void @test() {
+  entry:
+    ret void
+  }
+...
+
+---
+name:            test
+body: |
+  bb.0.entry:
+
+    ; CHECK: STDPtrQRr $r31r30, 10, $r9
+    ; CHECK-NEXT: STDPtrQRr $r31r30, 11, $r10
+
+    STDWSPQRr $sp, 10, $r10r9, implicit-def $sp
+...

llvm/test/CodeGen/AVR/regression/D114611.ll

+; RUN: llc < %s -march=avr | FileCheck %s
+
+declare { } @foo(i128, i128) addrspace(1)
+
+define i128 @bar(i128 %a, i128 %b) addrspace(1) {
+  call void asm "nop", "" ()
+; CHECK:      nop
+; CHECK-NEXT: ;NO_APP
+; CHECK-NEXT: Y+15, r20
+; CHECK-NEXT: Y+16, r21
+  %b_neg = icmp slt i128 %b, 0
+  %divisor = select i1 %b_neg, i128 0, i128 %b
+  %result = tail call fastcc addrspace(1) { } @foo(i128 undef, i128 %divisor)
+
+  ret i128 0
+}

llvm/utils/UpdateTestChecks/asm.py

 def get_triple_from_march(march):
   triples = {
       'amdgcn': 'amdgcn',
+      'avr': 'avr',
       'r600': 'r600',
       'mips': 'mips',
       'sparc': 'sparc',