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

[Jinsong Ji via Phabricator via llvm-commits]

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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73692