[PATCH] D96351: [AIX] Enable the default AltiVec ABI on AIX

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I personally don't feel that the included test case changes are adequate for testing this patch. First of all, the test case probably does too much without equivalent checks - there is 32/64 bit, default/extended ABI and MIR/ASM testing all in a single file and the checks for default/extended at least do not appear to line up. There is no explicit checks that no spills/reloads exist for the reserved regs with default ABI.

Furthermore, I think you should have a test case where you clobber all FPR's and VSR's **except** the ones in the `vr20 - vr31` range in functions that perform FP/vector computations with checks that show the affected registers are not allocated.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15164
+  if (Subtarget.isAIXABI() && !TM.getAIXExtendedAltivecABI()) {
+    if (R.first >= (PPC::V0 + 20) && R.second == &PPC::VSRCRegClass)
+      errs() << "warning: vector registers 20 to 32 are reserved in the "
----------------
Seems like this should be producing a warning for a range of registers, but the condition covers an unterminated range. That seems odd and potentially wrong. Why is this not:
```
if ((R.first >= PPC::V20 && R.first <= PPC::V31) ||
    (R.first >= PPC::VF20 && R.first <= PPC::VF31))
```


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:348
+
+      markSuperRegs(Reserved, CSR_Altivec_SaveList[I]);
+    }
----------------
Since this effectively shrinks the register class by 12 registers, it is worth double checking that marking these registers as reserved without providing an alternate allocation order does not adversely affect register pressure estimates.


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

https://reviews.llvm.org/D96351