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

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay marked 2 inline comments as done.
dylanmckay 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[] = {
----------------
rodrigorc wrote:
> 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.
Forgot about that one, nice.


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


================
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]>;
----------------
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.


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

https://reviews.llvm.org/D68524