[PATCH] D127530: [PowerPC] Extend GlobalISel implementation to emit and/or/xor.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:41
+    MIB.addUse(PhysReg, RegState::Implicit);
+    Register ExtReg = extendRegister(ValVReg, VA);
+    MIRBuilder.buildCopy(PhysReg, ExtReg);
----------------
Is it not possible to add tests at this time that actually extend the return value from `i32` (for both `sixgnext, zeroext`)?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:75-80
+    ArgInfo OrigArg{VRegs, Val->getType(), 0};
+    setArgFlags(OrigArg, AttributeList::ReturnIndex, DL, F);
+    splitToValueTypes(OrigArg, SplitArgs, DL, F.getCallingConv());
+    OutgoingValueAssigner ArgAssigner(RetCC_PPC);
+    OutgoingArgHandler ArgHandler(MIRBuilder, MRI, MIB);
+    Success = determineAndHandleAssignments(ArgHandler, ArgAssigner, SplitArgs,
----------------
I understand that you implemented this by looking at other targets that already have this implemented. However, I think it would help other PPC developers (as well as anyone implementing this for a future target) if we add a short comment before each statement here describing what the function call does or what the purpose of the object is.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCGenRegisterBankInfo.def:10
+/// This file defines all the static objects used by PPCRegisterBankInfo.
+/// \todo This should be generated by TableGen.
+//===----------------------------------------------------------------------===//
----------------
Can you please add a comment as to why it is not yet auto generated?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCGenRegisterBankInfo.def:26-28
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
----------------
Why are there 3 entries here? Thorough documentation at this stage will make it significantly easier when the time comes to refactor the implementation and add automatic generation, etc.


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

https://reviews.llvm.org/D127530