[PATCH] D121672: [AVR] Reject/Reserve R0~R15 on AVRTiny

[Ayke via Phabricator via llvm-commits]

aykevl added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRCallingConv.td:39
 def CSR_Normal : CalleeSavedRegs<(add R29, R28, (sequence "R%u", 17, 2))>;
+def CSR_NormalTiny : CalleeSavedRegs<(add R29, R28, R19, R18)>;
 def CSR_Interrupts : CalleeSavedRegs<(add(sequence "R%u", 31, 2))>;
----------------
I don't think this is needed. Reserved registers are not part of the regular calling convention anyway.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1056-1059
+  const MCPhysReg *RegList8 = Tiny ? RegList8Tiny : RegList8AVR;
+  const MCPhysReg *RegList16 = Tiny ? RegList16Tiny : RegList16AVR;
+  const unsigned RegAmount =
+      Tiny ? array_lengthof(RegList8Tiny) : array_lengthof(RegList8AVR);
----------------
I think it would make sense to use a vector here, to make the code safer. Take a look here to see the recommended vector types for use in LLVM: https://llvm.org/docs/ProgrammersManual.html#sequential-containers-std-vector-std-list-etc


================
Comment at: llvm/lib/Target/AVR/AVRRegisterInfo.cpp:60-62
   Reserved.set(AVR::R0);
   Reserved.set(AVR::R1);
   Reserved.set(AVR::R1R0);
----------------
I think it would make sense to change these hardcoded `R0`/`R1`/`R1R0` registers to properties of `AVRSubtarget` (a bit like `getIORegisterOffset` for example). This would make the code below a bit more sensible: it could just reserve `R0`-`R15` instead of `R2`-`R17` which looks a bit strange.
Such a change should probably need to be done in a separate patch, as a cleanup before this patch. Such a patch could also fix the hardcoded `R0`/`R1` registers in AVRFrameLowering.cpp, AVRExpandPseudoInsts.cpp, etc.


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

https://reviews.llvm.org/D121672