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

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added a comment.

I've had a quick first pass, nice work, I will have a deeper look tonight.



================
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[] = {
----------------
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)


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:928
+    // Round up to even number of bytes.
+    TotalBytes = (TotalBytes + 1) / 2 * 2;
+    // Skip zero sized arguments
----------------
Recommend using `alignTo` from `Support/MathExtras.h`

This expression is equivalent to `alignTo(TotalBytes, 2);


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


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:936
+    // If there is no enough registers, use the stack
+    if (RegIdx >= array_lengthof(RegList8)) {
+      UseStack = true;
----------------
Add explicit `#include` for `ADT/STLExtras.h`


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:960
+        CCInfo.addLoc(CCValAssign::getReg(i, VT, Reg, VT, CCValAssign::Full));
+        // Registers inside a particular argument are sort in increasing order
+        // (remember the array is reversed).
----------------
`s/sort/sorted`


================
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,
----------------
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.


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:987
   } else {
-    analyzeStandardArguments(&CLI, F, TD, Outs, Ins,
-                             CallConv, ArgLocs, CCInfo,
-                             IsCall, IsVarArg);
-  }
-}
-
-static void analyzeArguments(TargetLowering::CallLoweringInfo *CLI,
-                             const Function *F, const DataLayout *TD,
-                             const SmallVectorImpl<ISD::OutputArg> *Outs,
-                             const SmallVectorImpl<ISD::InputArg> *Ins,
-                             CallingConv::ID CallConv,
-                             SmallVectorImpl<CCValAssign> &ArgLocs,
-                             CCState &CCInfo, bool IsCall, bool IsVarArg) {
-  switch (CallConv) {
-    case CallingConv::AVR_BUILTIN: {
-      analyzeBuiltinArguments(*CLI, F, TD, Outs, Ins,
-                              CallConv, ArgLocs, CCInfo,
-                              IsCall, IsVarArg);
-      return;
-    }
-    default: {
-      analyzeStandardArguments(CLI, F, TD, Outs, Ins,
-                               CallConv, ArgLocs, CCInfo,
-                               IsCall, IsVarArg);
-      return;
+    TotalBytes = (TotalBytes + 1) / 2 * 2;
+  }
----------------
Same comment as above re. `alignTo`


================
Comment at: lib/Target/AVR/AVRISelLowering.cpp:1321
 
-bool
-AVRTargetLowering::CanLowerReturn(CallingConv::ID CallConv,
-                                  MachineFunction &MF, bool isVarArg,
-                                  const SmallVectorImpl<ISD::OutputArg> &Outs,
-                                  LLVMContext &Context) const
-{
-  SmallVector<CCValAssign, 16> RVLocs;
-  CCState CCInfo(CallConv, isVarArg, MF, RVLocs, Context);
-
-  auto CCFunction = CCAssignFnForReturn(CallConv);
-  return CCInfo.CheckReturn(Outs, CCFunction);
+  unsigned TotalBytes = 0;
+  unsigned NumArgs = Outs.size();
----------------
This for loop summing up the sizes of all arguments has also exists on line 974 - recommend extracting it into a function, something like `unsigned getTotalArgumentsSizeInBytes(args)`


================
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)
----------------
Good catch, thanks for the bugfix!


================
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]>;
----------------
Can we give these registers a common prefix to indicate their pseudoness, something like `PR26R25`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68524