[PATCH] D73692: [PowerPC] Fix spilling of vector registers in PEI of EH aware functions

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 6 inline comments as done.
nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCFrameLowering.cpp:2240
+            MF->getFunction().hasFnAttribute(Attribute::NoUnwind) ||
+            Subtarget.hasP9Vector())
+          TII.storeRegToStackSlot(MBB, MI, Reg, !IsLiveIn, CSI[i].getFrameIdx(),
----------------
nemanjai wrote:
> jsji wrote:
> > stefanp wrote:
> > > nit:
> > > This condition here in `spillCalleeSavedRegisters` needs to be identical to the one in `restoreCalleeSavedRegisters`. To avoid possible future bugs I would create a function to test for this and call it in both places. That would avoid a situation where someone adds a condition here and forgets to add it to `restoreCalleeSavedRegisters`.
> > > 
> > > Something like?
> > > ```
> > > bool PPCFrameLowering::mustPreserveElementOrder(MachineFunction *MF) {
> > >   return Subtarget.isLittleEndian() && !MF->getFunction().hasFnAttribute(Attribute::NoUnwind) && !Subtarget.hasP9Vector();
> > > }
> > > ```
> > Agree.
> Ah yes, great suggestion. Thanks.
Turns out we have a function that tests a couple of these already. I don't know why I forgot it since it was me who originally added it :)

The condition now is probably to simple to pull out into a separate function:
```
        if (Subtarget.needsSwapsForVSXMemOps() &&
            !MF->getFunction().hasFnAttribute(Attribute::NoUnwind))
          TII.storeRegToStackSlotNoUpd(MBB, MI, Reg, !IsLiveIn,
                                       CSI[i].getFrameIdx(), RC, TRI);
        else
          TII.storeRegToStackSlot(MBB, MI, Reg, !IsLiveIn, CSI[i].getFrameIdx(),
                                  RC, TRI);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1321
+  if (Subtarget.hasVSX() && RC == &PPC::VRRCRegClass)
+    RC = &PPC::VSRCRegClass;
+
----------------
nemanjai wrote:
> stefanp wrote:
> > nit:
> > We can probably use `RC = updatedRC(RC);` like we do in `PPCInstrInfo::storeRegToStackSlot`.
> > 
> Yeah. I didn't change this though. That's how it was.
Decided to change it anyway.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:293
 
+  void storeRegToStackSlotNoUpd(MachineBasicBlock &MBB,
+                                MachineBasicBlock::iterator MBBI,
----------------
nemanjai wrote:
> jsji wrote:
> > NeHuang wrote:
> > > nit: IMHO it could be better to add descriptions/comments on `NoUpd` for the two new functions `storeRegToStackSlotNoUpd` and  `loadRegFromStackSlotNoUpd`? 
> > Nit: `NoUpd` here is referring to `No updatedRC`? It might be confusing at first glance, as store with update may refer to store with index update?
> > 
> > Why we want to create a new interface here? Why not extending `storeRegToStackSlot` with a default arg?
> > `storeRegToStackSlot(... , bool mustPreserveElementOrder=true)`
> > `loadRegFromStackSlot(...., bool mustPreserveElementOrder=true)`
> I can't really say that I'm up on my knowledge of how virtual function signatures are allowed or not allowed to change between the base class and the derived class. But I can try it and see if the compiler complains.
OK, tried it and apparently you can't change the signature of virtual functions you override. Which I initially assumed without trying it, now I've confirmed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73692