This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix spilling of vector registers in PEI of EH aware functions
ClosedPublic

Authored by nemanjai on Jan 30 2020, 4:55 AM.

Details

Summary

On little endian targets prior to Power9, we spill vector registers using a
swapping store (i.e. stdxvd2x saves the vector with the two doublewords in
big endian order regardless of endianness). This is generally not a problem
since we restore them using the corresponding swapping load (lxvd2x). However
if the restore is done by the unwinder, the vector register contains data in
the incorrect order.

This patch fixes that by using Altivec loads/stores for vector saves and
restores in PEI (which keep the order correct) under those specific conditions:

  • Little endian target
  • EH aware function
  • Subtarget prior to Power9 (which has non-swapping loads/stores)

Diff Detail

Event Timeline

nemanjai created this revision.Jan 30 2020, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2020, 4:55 AM

Unit tests: fail. 62335 tests passed, 1 failed and 838 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: fail. clang-tidy found 0 errors and 3 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

nemanjai added a project: Restricted Project.Jan 30 2020, 7:53 AM

The unit test failure is due to an unrelated commit to libc++. I'm not sure why it is showing up here, but presumably when I upload any new updates and the bot re-runs the tests, that should go away.

stefanp accepted this revision as: stefanp.Feb 5 2020, 12:12 PM
stefanp added a subscriber: stefanp.

Couple of nits but overall LGTM.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2240

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();
}
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
1321

nit:
We can probably use RC = updatedRC(RC); like we do in PPCInstrInfo::storeRegToStackSlot.

This revision is now accepted and ready to land.Feb 5 2020, 12:12 PM
NeHuang added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.h
293

nit: IMHO it could be better to add descriptions/comments on NoUpd for the two new functions storeRegToStackSlotNoUpd and loadRegFromStackSlotNoUpd?

305

same as 293

jsji added inline comments.Feb 5 2020, 12:58 PM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2240

Agree.

llvm/lib/Target/PowerPC/PPCInstrInfo.h
293

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)

The unit test failure should not relate to this patch. Saw the same failure in https://reviews.llvm.org/D68237. (it passed in the latest run today)

nemanjai marked 3 inline comments as done.Feb 7 2020, 6:53 AM

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

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2240

Ah yes, great suggestion. Thanks.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
1321

Yeah. I didn't change this though. That's how it was.

llvm/lib/Target/PowerPC/PPCInstrInfo.h
293

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.

nemanjai marked 6 inline comments as done.Feb 7 2020, 12:20 PM
nemanjai added inline comments.
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2240

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);
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
1321

Decided to change it anyway.

llvm/lib/Target/PowerPC/PPCInstrInfo.h
293

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.

This revision was automatically updated to reflect the committed changes.
jsji added inline comments.Feb 7 2020, 12:50 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.h
293

Yes, my original proposal is not to change it when override it. But to change the original virtual functions as well. But anyway, this can be a nice to have NFC.