[PATCH] D114611: [AVR] Expand STDWSPQRr & STDSPQRr, approach #2

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay requested changes to this revision.
dylanmckay added a comment.
This revision now requires changes to proceed.

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



================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:2113
+  MI.setDesc(TII->get(AVR::STDPtrQRr));
+  MI.getOperand(0).setReg(STI.getFrameLowering()->hasReservedCallFrame(MF) ? AVR::R29R28 : AVR::R31R30);
+
----------------
Recommend you split the logic `STI.getFrameLowering()->hasReservedCallFrame(MF) ? AVR::R29R28 : AVR::R31R30` into a method, with some reasonable name like `getStackStoreBaseRegister


================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:2121
+  MachineInstr &MI = *MBBI;
+  MachineFunction &MF = *MI.getParent()->getParent();
+  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
----------------
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()`





================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:2204
     EXPAND(AVR::SPWRITE);
+    EXPAND(AVR::STDSPQRr);
+    EXPAND(AVR::STDWSPQRr);
----------------
Minor: Move this above next to `STDWPtrQRr` so similar operations are similarly ordered


================
Comment at: llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.ll:1
+; RUN: llc < %s -march=avr | FileCheck %s
+
----------------
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.


================
Comment at: llvm/test/CodeGen/AVR/pseudo/STDWSPQRr.ll:6
+define i128 @bar(i128 %a, i128 %b) addrspace(1) {
+; CHECK-LABEL: LBB0_6:
+; CHECK-NEXT:    std Y+13, r18
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114611/new/

https://reviews.llvm.org/D114611