[llvm-commits] [llvm] r156458 - in /llvm/trunk: lib/Target/Mips/MipsRegisterInfo.cpp test/CodeGen/Mips/fp-spill-reload.ll

[Hatanaka, Akira]

The current Mips target architectures do not have large vector registers, and the stack alignments of O32 and N32/64 ABIs are at least as large as the size of the largest register. So for now, dynamic stack alignment is not needed to spill registers. 

Thanks again for your review.

________________________________________
From: Jim Grosbach [grosbach at apple.com]
Sent: Wednesday, May 09, 2012 4:25 PM
To: Hatanaka, Akira
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [llvm] r156458 - in /llvm/trunk: lib/Target/Mips/MipsRegisterInfo.cpp test/CodeGen/Mips/fp-spill-reload.ll

On May 9, 2012, at 4:20 PM, Hatanaka, Akira wrote:

> Hi Jim,
>
> Thank you for the review and comment.
>
> Currently, the Mips backend does not do any dynamic stack realignment.
> I don't fully understand when dynamic stack realignment is needed for register spills, but I don't think the current Mips targets need it.

It probably doesn't need it. On ARM and X86 it happens when the spill slots for some register class (vector registers in those cases) require alignment greater than the ABI stack alignment.

>
> My intention was to reserve FP whenever it is used as a dedicated frame pointer. I didn't consider cases in which a function without variable sized allocas can have a frame pointer. As you pointed out, the comment is misleading. I will fix it later.
>

Cool. Thanks!

And once again, I'm really excited to see the Mips back end making so much progress. This is great stuff. :)

-Jim

> ________________________________________
> From: Jim Grosbach [grosbach at apple.com]
> Sent: Wednesday, May 09, 2012 10:57 AM
> To: Hatanaka, Akira
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [llvm] r156458 - in /llvm/trunk: lib/Target/Mips/MipsRegisterInfo.cpp test/CodeGen/Mips/fp-spill-reload.ll
>
> Hi Akira,
>
> Does the Mips backend ever do dynamic stack realignment for register spills? If so, there are additional complexities to consider as whether the FP is required can change after regalloc. ARM and X86 run into that problem, for example.
>
> One nitpick below.
>
> Thanks,
>  Jim
>
> On May 8, 2012, at 6:38 PM, Akira Hatanaka wrote:
>
>> Author: ahatanak
>> Date: Tue May  8 20:38:13 2012
>> New Revision: 156458
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=156458&view=rev
>> Log:
>> Make register FP allocatable if the compiled function does not have dynamic
>> allocas.
>>
>>
>> Added:
>>   llvm/trunk/test/CodeGen/Mips/fp-spill-reload.ll
>> Modified:
>>   llvm/trunk/lib/Target/Mips/MipsRegisterInfo.cpp
>>
>> Modified: llvm/trunk/lib/Target/Mips/MipsRegisterInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsRegisterInfo.cpp?rev=156458&r1=156457&r2=156458&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/Mips/MipsRegisterInfo.cpp (original)
>> +++ llvm/trunk/lib/Target/Mips/MipsRegisterInfo.cpp Tue May  8 20:38:13 2012
>> @@ -85,12 +85,12 @@
>> getReservedRegs(const MachineFunction &MF) const {
>>  static const uint16_t ReservedCPURegs[] = {
>>    Mips::ZERO, Mips::AT, Mips::K0, Mips::K1,
>> -    Mips::SP, Mips::FP, Mips::RA
>> +    Mips::SP, Mips::RA
>>  };
>>
>>  static const uint16_t ReservedCPU64Regs[] = {
>>    Mips::ZERO_64, Mips::AT_64, Mips::K0_64, Mips::K1_64,
>> -    Mips::SP_64, Mips::FP_64, Mips::RA_64
>> +    Mips::SP_64, Mips::RA_64
>>  };
>>
>>  BitVector Reserved(getNumRegs());
>> @@ -124,6 +124,12 @@
>>    Reserved.set(Mips::GP_64);
>>  }
>>
>> +  // If this function has dynamic allocas, reserve FP.
>> +  if (MF.getTarget().getFrameLowering()->hasFP(MF)) {
>
> The code doesn't seem to, at least directly, follow from the comment. Based on the comment I'd have expected to see MFI->hasVarSizedObjects(). That said, I think it's the comment that's misleading, not the code. Are there ever cases where the function has a frame pointer, whether vararray related or not, that the FP shouldn't be reserved? I would think not, so that's probably what the comment say and not reference vararrays at all.
>
>> +    Reserved.set(Mips::FP);
>> +    Reserved.set(Mips::FP_64);
>> +  }
>> +
>>  // Reserve hardware registers.
>>  Reserved.set(Mips::HWR29);
>>  Reserved.set(Mips::HWR29_64);
>>
>> Added: llvm/trunk/test/CodeGen/Mips/fp-spill-reload.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/fp-spill-reload.ll?rev=156458&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/Mips/fp-spill-reload.ll (added)
>> +++ llvm/trunk/test/CodeGen/Mips/fp-spill-reload.ll Tue May  8 20:38:13 2012
>> @@ -0,0 +1,39 @@
>> +; RUN: llc -march=mipsel < %s | FileCheck %s
>> +; check that $fp is not reserved.
>> +
>> +define void @foo0(i32* nocapture %b) nounwind {
>> +entry:
>> +; CHECK: sw  $fp
>> +; CHECK: lw  $fp
>> +  %0 = load i32* %b, align 4
>> +  %arrayidx.1 = getelementptr inbounds i32* %b, i32 1
>> +  %1 = load i32* %arrayidx.1, align 4
>> +  %add.1 = add nsw i32 %1, 1
>> +  %arrayidx.2 = getelementptr inbounds i32* %b, i32 2
>> +  %2 = load i32* %arrayidx.2, align 4
>> +  %add.2 = add nsw i32 %2, 2
>> +  %arrayidx.3 = getelementptr inbounds i32* %b, i32 3
>> +  %3 = load i32* %arrayidx.3, align 4
>> +  %add.3 = add nsw i32 %3, 3
>> +  %arrayidx.4 = getelementptr inbounds i32* %b, i32 4
>> +  %4 = load i32* %arrayidx.4, align 4
>> +  %add.4 = add nsw i32 %4, 4
>> +  %arrayidx.5 = getelementptr inbounds i32* %b, i32 5
>> +  %5 = load i32* %arrayidx.5, align 4
>> +  %add.5 = add nsw i32 %5, 5
>> +  %arrayidx.6 = getelementptr inbounds i32* %b, i32 6
>> +  %6 = load i32* %arrayidx.6, align 4
>> +  %add.6 = add nsw i32 %6, 6
>> +  %arrayidx.7 = getelementptr inbounds i32* %b, i32 7
>> +  %7 = load i32* %arrayidx.7, align 4
>> +  %add.7 = add nsw i32 %7, 7
>> +  call void @foo2(i32 %0, i32 %add.1, i32 %add.2, i32 %add.3, i32 %add.4, i32 %add.5, i32 %add.6, i32 %add.7) nounwind
>> +  call void bitcast (void (...)* @foo1 to void ()*)() nounwind
>> +  call void @foo2(i32 %0, i32 %add.1, i32 %add.2, i32 %add.3, i32 %add.4, i32 %add.5, i32 %add.6, i32 %add.7) nounwind
>> +  ret void
>> +}
>> +
>> +declare void @foo2(i32, i32, i32, i32, i32, i32, i32, i32)
>> +
>> +declare void @foo1(...)
>> +
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>