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

[Ehsan Amiri via llvm-commits]

amehsan added a comment.

Regarding (2): I think we need to make sure InstCombine and target Independent lowering are doing the right thing for these.  Fixing as late as possible does not seem correct to me. But I leave to other reviewers to decide.



================
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:
> nemanjai wrote:
> > 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.
> We don't know that the COPY is guaranteed to be removed. AFAIK, We should not rely on downstream optimizations if we can generate better code. Yes, all uses should be replaced. There is a utility method for that: Check: include/llvm/CodeGen/MachineRegisterInfo.h for method:
>   void replaceRegWith(unsigned FromReg, unsigned ToReg)
Looking at this method again, it has differences with replaceAllUsesWith. First I thought it is the same. Some comment from people with more experience will be helpful.


Repository:
  rL LLVM

https://reviews.llvm.org/D26066