Missed in the original commit, use the correct callee-saved register
list for spilling, instead of the standard SVR432 list. This avoids
needlessly spilling the SPE non-volatile registers when they're not used.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 26834 Build 26833: arc lint + arc unit
Event Timeline
Comment Actions
I think this is very close, but I really think the reloading code needs to be fixed.
| lib/Target/PowerPC/PPCInstrInfo.cpp | ||
|---|---|---|
| 1161 | This does not look right. We are in RC == nullptr section here. Why are we checking hasSubClassEq(nullptr)? | |
| lib/Target/PowerPC/PPCRegisterInfo.cpp | ||
| 163 | These ternary operator expressions were arguably too complex already. I don't think there's any doubt that after this they're becoming nearly unparsable by a human. Can you please refactor these to use nested if's? | |
Comment Actions
Is there a good example for a test case testing register spills with no determined register class? That's the only location where I added new spill handling. The real meat of this should already be handled by the existing regression tests.
Comment Actions
I removed the extra spill changes. Those can be done later if there actually is a problem. This didn't change the tests at all. I'm not quite sure how to write a test to demonstrate that "some but not all" registers are spilled in a given case, given that the register allocator can change at any time, so can't hard-code the registers that would get spilled, and I don't think I can even hard-code the number of registers that could get spilled.
Comment Actions
LGTM other than the code here looks really messy - but it looks messy regardless of this patch.
I think after this lands, we should refactor this stuff to something like this:
const MCPhysReg *CSRLists[] = {
CSR_64_AllRegs_VSX_SaveList,
CSR_64_AllRegs_Altivec_SaveList,
CSR_64_AllRegs_SaveList,
CSR_Darwin64_Altivec_SaveList,
CSR_Darwin64_SaveList,
CSR_Darwin32_Altivec_SaveList,
CSR_Darwin32_SaveList,
...
};
// Compute the index into the array based on Subtarget features, calling convention, ABI, the need to save R2, and object mode (32/64 bit).
return CSRLists[ComputedIdx];and similarly for the regmasks.
| lib/Target/PowerPC/PPCRegisterInfo.cpp | ||
|---|---|---|
| 174 | No need for else when the previous if just does a return. The same comment applies to all the else statements here. // Cold calling convention CSR's.
if (MF->getFunction().getCallingConv() == CallingConv::Cold) {
// 64-bit targets.
if (TM.isPPC64()) {
if (Subtarget.hasAltivec())
return SaveR2 ? ...
return SaveR2 ? ...
}
// 32-bit targets.
if (Subtarget.hasAltivec())
return CSR_SVR32_ColdCC_Altivec_SaveList;
if (Subtarget.hasSPE())
return CSR_SVR32_ColdCC_SPE_SaveList;
return CSR_SVR32_ColdCC_SaveList
}
// Standard calling convention CSR's.
... | |
This does not look right. We are in RC == nullptr section here. Why are we checking hasSubClassEq(nullptr)?