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

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay marked an inline comment as done.
dylanmckay added a comment.

@Sh4rK your review is always welcome - I've updated the code with your comments, here's the delta containing just the fixes you've suggested.

  patch
  diff --git a/llvm/lib/Target/AVR/AVRISelLowering.cpp b/llvm/lib/Target/AVR/AVRISelLowering.cpp
  index 20e9d209531..bf9b32e1278 100644
  --- a/llvm/lib/Target/AVR/AVRISelLowering.cpp
  +++ b/llvm/lib/Target/AVR/AVRISelLowering.cpp
  @@ -896,7 +896,7 @@ static const MCPhysReg RegList16[] = {
       AVR::R10R9,  AVR::R9R8};
   
   static_assert(array_lengthof(RegList8) == array_lengthof(RegList16),
  -        "8-bit and 15-bit register arrays must be of equal length");
  +        "8-bit and 16-bit register arrays must be of equal length");
   
   /// Analyze incoming and outgoing function arguments. We need custom C++ code
   /// to handle special constraints in the ABI.
  @@ -972,11 +972,9 @@ analyzeArguments(TargetLowering::CallLoweringInfo *CLI, const Function *F,
   template <typename ArgT>
   static unsigned getTotalArgumentsSizeInBytes(const SmallVectorImpl<ArgT> &Args) {
     unsigned TotalBytes = 0;
  -  unsigned NumArgs = Args.size();
   
  -  for (unsigned i = 0; i != NumArgs; ++i) {
  -    MVT VT = Args[i].VT;
  -    TotalBytes += VT.getStoreSize();
  +  for (const ArgT& Arg : Args) {
  +    TotalBytes += Arg.VT.getStoreSize();
     }
     return TotalBytes;
   }
  @@ -990,7 +988,7 @@ static void analyzeReturnValues(const SmallVectorImpl<ArgT> &Args,
     unsigned NumArgs = Args.size();
     unsigned TotalBytes = getTotalArgumentsSizeInBytes(Args);
     // CanLowerReturn() guarantees this assertion.
  -  assert(TotalBytes <= 8 && "return values greter than 8 bytes cannot be lowered");
  +  assert(TotalBytes <= 8 && "return values greater than 8 bytes cannot be lowered");
   
     // GCC-ABI says that the size is rounded up to the next even number,
     // but actually once it is more than 4 it will always round up to 8.
  diff --git a/llvm/lib/Target/AVR/AVRRegisterInfo.td b/llvm/lib/Target/AVR/AVRRegisterInfo.td
  index 8e36971dd63..ab5d02356c9 100644
  --- a/llvm/lib/Target/AVR/AVRRegisterInfo.td
  +++ b/llvm/lib/Target/AVR/AVRRegisterInfo.td
  @@ -103,7 +103,8 @@ CoveredBySubRegs = 1 in
     def R5R4   : AVRReg<4,  "r5:r4",   [R4, R5]>,   DwarfRegNum<[4]>;
     def R3R2   : AVRReg<2,  "r3:r2",   [R2, R3]>,   DwarfRegNum<[2]>;
     def R1R0   : AVRReg<0,  "r1:r0",   [R0, R1]>,   DwarfRegNum<[0]>;
  -  // Pseudo registers for unaligned i32
  +
  +  // Pseudo registers for unaligned i16
     def R26R25 : AVRReg<25, "r26:r25", [R25, R26]>, DwarfRegNum<[25]>;
     def R24R23 : AVRReg<23, "r24:r23", [R23, R24]>, DwarfRegNum<[23]>;
     def R22R21 : AVRReg<21, "r22:r21", [R21, R22]>, DwarfRegNum<[21]>;



================
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:
> rodrigorc wrote:
> > 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?
> That is indeed a weird one - the answer should lie inside `llvm/utils/TableGen/RegisterInfoEmitter.cpp`.
> 
> The `.td` format was definitely the hardest thing for me to understand when I first got into LLVM.
> 
> I don't think it should do that.
This comment is minor IMO, giving the registers a common prefix is unnecessary for now.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:901
+static_assert(array_lengthof(RegList8) == array_lengthof(RegList16),
+        "8-bit and 15-bit register arrays must be of equal length");
 
----------------
Sh4rK wrote:
> **16**-bit
Good catch, have fixed.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:979
+
+  for (unsigned i = 0; i != NumArgs; ++i) {
+    MVT VT = Args[i].VT;
----------------
Sh4rK wrote:
> This could be replaced with a range-based for loop, the LLVM coding standards seems to encourage this as well.
Good idea, I agree, fixed.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:995
+  // CanLowerReturn() guarantees this assertion.
+  assert(TotalBytes <= 8 && "return values greter than 8 bytes cannot be lowered");
+
----------------
Sh4rK wrote:
> s/greter/greater
Fixed


================
Comment at: llvm/lib/Target/AVR/AVRRegisterInfo.td:106
   def R1R0   : AVRReg<0,  "r1:r0",   [R0, R1]>,   DwarfRegNum<[0]>;
+  // Pseudo registers for unaligned i32
+  def R26R25 : AVRReg<25, "r26:r25", [R25, R26]>, DwarfRegNum<[25]>;
----------------
Sh4rK wrote:
> I'm not entirely sure, but I think **i32** was meant to be **i16**.
I believe you are right, fixed.


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

https://reviews.llvm.org/D68524