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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:78-88
+static const TargetRegisterClass *guessRegClass(unsigned Reg,
+                                                MachineRegisterInfo &MRI,
+                                                const TargetRegisterInfo &TRI,
+                                                const RegisterBankInfo &RBI) {
+  const RegisterBank *RegBank = RBI.getRegBank(Reg, MRI, TRI);
+  assert(RegBank && "Can't get reg bank for virtual register");
+
----------------
I don't see the point of this function. There's no guessing involved and you're returning a static result anyway


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:94
+  Register DstReg = I.getOperand(0).getReg();
+  if (Register::isPhysicalRegister(DstReg))
+    return true;
----------------
You still need to at least constrain the source register if it's virtual. You probably should have some MIR tests for every permutation of physical / virtual register copies


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:84-87
+    if ((Register::isPhysicalRegister(DstReg) ||
+         !MRI.getType(DstReg).isValid()) ||
+        (Register::isPhysicalRegister(SrcReg) ||
+         !MRI.getType(SrcReg).isValid())) {
----------------
You shouldn't need to guard these. getRegBank should do the right thing


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

https://reviews.llvm.org/D127530