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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 3 inline comments as done.
nemanjai added a comment.

Thanks for the comments. I'll update the patch prior to committing.



================
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(),
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1321
+  if (Subtarget.hasVSX() && RC == &PPC::VRRCRegClass)
+    RC = &PPC::VSRCRegClass;
+
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:293
 
+  void storeRegToStackSlotNoUpd(MachineBasicBlock &MBB,
+                                MachineBasicBlock::iterator MBBI,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73692