[PATCH] D26066: [PowerPC] Improvements for BUILD_VECTOR Vol. 4

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:151-154
+                BuildMI(MBB, &MI, MI.getDebugLoc(),
+                        TII->get(PPC::COPY), MI.getOperand(0).getReg())
+                  .addOperand(MI.getOperand(1));
+                ToErase = &MI;
----------------
amehsan wrote:
> Can't we just replace uses of MI.getOperand(0) with MI.getOperand(1) instead of creating a new copy instruction? 
This is safe to do. The COPY will be removed if not needed. I did something similar before and it caused a problem when the operand defined by the removed instruction had another use. Please see https://reviews.llvm.org/D25493.
I suppose I could traverse the use list for MI.getOperand(0) and see if it can be replaced as well, but I don't think that's any cleaner.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:244-246
+          (MyOpcode == PPC::XXSPLTW && DefOpcode == PPC::LXVWSX) ||
+          (MyOpcode == PPC::XXSPLTW && DefOpcode == PPC::MTVSRWS)||
+          (MyOpcode == PPC::XXSPLTW && isConvertOfSplat());
----------------
amehsan wrote:
> Given the name of variable being defined 'SameOpcode' these three lines that you have added requires some comment explaining what's going on. 
Good point. I'll rename the variable and add a short comment.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:304-334
+            if (P1->getOpcode() == PPC::FRSP &&
+                MRI->hasOneNonDBGUse(P1->getOperand(0).getReg())) {
+              unsigned ConvReg1 = P1->getOperand(1).getReg();
+              unsigned FRSP1Defines = P1->getOperand(0).getReg();
+              for (MachineInstr &I : MRI->use_instructions(FRSP1Defines)) {
+                for (int i = 0, e = I.getNumOperands(); i < e; i++)
+                  if (I.getOperand(i).isReg() &&
----------------
amehsan wrote:
> I think you can factor out the common logic here either in a lambda or a helper function. You are doing very similar things for P1 and P2.
That's a great idea. I'll do that.


Repository:
  rL LLVM

https://reviews.llvm.org/D26066