[PATCH] D68524: [AVR] Rewrite the function calling convention.

[Rodrigo Rivas Costa via Phabricator via llvm-commits]

rodrigorc planned changes to this revision.
rodrigorc added inline comments.


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:887
+/// Registers for calling conventions, ordered in reverse as required by ABI.
+/// Both arrays must be of the same length.
+static const MCPhysReg RegList8[] = {
----------------
dylanmckay wrote:
> Can we add a `static_assert`ion that this invariant is upheld?
> 
> Something like
> 
> ```
> static_assert(sizeof(RegList8) * 2 == sizeof(RegList16));
> ```
> 
> It looks like LLVM supports static_assert [Coding Standards](http://releases.llvm.org/8.0.0/docs/CodingStandards.html#supported-c-11-language-and-library-features)
Indeed! But I'll use array_lengthof() instead of sizeof(), that looks nicer for this case.


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:935
+    RegLastIdx = RegIdx;
+    // If there is no enough registers, use the stack
+    if (RegIdx >= array_lengthof(RegList8)) {
----------------
dylanmckay wrote:
> `s/no/not`
Also s/is/are/, I think...


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:981
+  }
+  assert(TotalBytes <= 8);
+  // GCC-ABI says that the size is rounded up to the next even number,
----------------
dylanmckay wrote:
> Where does the 8-byte maximum retval size emerge from? Perhaps you could add some text to the assertion so that there will be more detail if ever hit.
AFAIUI, when `CanLowerReturn()` returns `false` this function will not be called. I'm adding a comment and a string to the `assert()`.


================
Comment at: lib/Target/AVR/AVRInstrInfo.cpp:51
   if (AVR::DREGSRegClass.contains(DestReg, SrcReg)) {
-    if (STI.hasMOVW()) {
+    if (STI.hasMOVW() && AVR::DREGSMOVWRegClass.contains(DestReg, SrcReg)) {
       BuildMI(MBB, MI, DL, get(AVR::MOVWRdRr), DestReg)
----------------
dylanmckay wrote:
> Good catch, thanks for the bugfix!
Actually, I'm not sure this bug could be triggered before, but now there are all these new pseudo-registers and this is really needed.


================
Comment at: lib/Target/AVR/AVRRegisterInfo.td:107
+  // Pseudo registers for unaligned i32
+  def R26R25 : AVRReg<25, "r26:r25", [R25, R26]>, DwarfRegNum<[25]>;
+  def R24R23 : AVRReg<23, "r24:r23", [R23, R24]>, DwarfRegNum<[23]>;
----------------
dylanmckay wrote:
> Can we give these registers a common prefix to indicate their pseudoness, something like `PR26R25`?
I tried that, but then somehow the testcase AVR/trunc.ll fails. I don't know exactly why, but it should generate the instr:

	mov	r24, r23

but instead it emits:

	mov	r23, r22
	mov	r24, r23

Looking around, I noticed that just by renaming these regs, the generated tables are quite different. That is, in AVRGenRegisterInfo.inc, the array `AVRRegDiffLists` goes from 22 entries to 57! I admit that I don't fully understand the .td format, but I think that just renaming the register should not do this, should it?


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

https://reviews.llvm.org/D68524