Hi, attached is a second try for the bigstack patch for SPU, with testcase. It is essentially the patch committed as 97091, and reverted as 97099, but with the following additions: -in vararg handling, registers are marked to be live, to not confuse the register scavenger -function prologue and epilogue are not emitted, if the stack size is 16. 16 means it is empty - there is only the register scavenger emergency spill slot, which is not used as there is no stack. This time there are no new unexpected failures in the regression tests. kalle -------------- next part -------------- A non-text attachment was scrubbed... Name: spu_bigstack.patch Type: text/x-patch Size: 10522 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100329/ff3ae124/attachment.bin> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: bigstack.ll URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100329/ff3ae124/attachment.ksh>
works for me, applied here, thanks! http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100329/098759.html On Mar 29, 2010, at 6:50 AM, Kalle Raiskila wrote:> Hi, > > attached is a second try for the bigstack patch for SPU, with testcase. It is essentially the patch committed as 97091, and reverted as 97099, but with the following additions: > -in vararg handling, registers are marked to be live, to not confuse the register scavenger > -function prologue and epilogue are not emitted, if the stack size is 16. 16 means it is empty - there is only the register scavenger emergency spill slot, which is not used as there is no stack. > > This time there are no new unexpected failures in the regression tests. > > > kalle > <spu_bigstack.patch><bigstack.ll>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Mar 29, 2010, at 6:50 AM, Kalle Raiskila wrote:> attached is a second try for the bigstack patch for SPU, with testcase. It is essentially the patch committed as 97091, and reverted as 97099, but with the following additions: > -in vararg handling, registers are marked to be live, to not confuse the register scavengerLooks good. You can try running with -verify-machineinstrs to detect more issues like that.> -function prologue and epilogue are not emitted, if the stack size is 16. 16 means it is empty - there is only the register scavenger emergency spill slot, which is not used as there is no stack.Would it be possible to detect this before allocating the emergency spill slot, and not request a scavenger at all?> This time there are no new unexpected failures in the regression tests.I noticed that CellSPU has a bunch of isS10Constant() functions. These already exist in MathExtras.h: isInt<10>(). Also, there is no need for 5 overloads of those functions, AFAICT. Please use the MathExtras.h functions, and remove all of the is*Constant functions in SPU.h. /jakob
Jakob Stoklund Olesen skrev:> Looks good. You can try running with -verify-machineinstrs to detect more > issues like that.Didn't know about that option, thanks for the tip. The output looks useful!>> -function prologue and epilogue are not emitted, if the stack size is 16. >> 16 means it is empty - there is only the register scavenger emergency spill >> slot, which is not used as there is no stack. > > Would it be possible to detect this before allocating the emergency spill > slot, and not request a scavenger at all?Initially I thought not, but heeding your previous advice and studying the ARM backend closely, I noticed the function estimateStackSize... I copied the function to the SPU backend, and enable the scavenger conditionally if stack is small enough. Initial tests look promising! The estimateStackSize looks quite generic (backend ignorant). Is there a reason it is in local to the ARM and not in TargetRegisterInfo class? Are there cases when the estimation would fail?> I noticed that CellSPU has a bunch of isS10Constant() functions. These > already exist in MathExtras.h: isInt<10>(). Also, there is no need for 5 > overloads of those functions, AFAICT.Hmm, I just mindlessly copied some legacy code. Thanks for the tip. I noticed that Benjamin Kramer already made the changes to the SPU backend :) kalle