[PATCH] D55444: AMDGPU: Fix DPP combiner

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin marked 4 inline comments as done.
vpykhtin added a comment.

Hi Nikolai,

thank you for the review, I'll think of how to restructure the code to match top comments.



================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:358
   assert(OldOpnd && OldOpnd->isReg());
   auto OldOpndVGPR = getRegSubRegPair(*OldOpnd);
   auto *OldOpndValue = getOldOpndValue(*OldOpnd);
----------------
nhaehnle wrote:
> Having both `OldOpndVGPR` and `OldOpndValue` seems redundant. You could pass `OldOpndValue` as a MachineOperand to `createDPPInst` and have it assert on `!CombinedOld || CombinedOld->Reg()`. That should help cut down the code size here somewhat and make the code easier to follow.
There was a reason I did it initially though after a time I need to recall it. If I remember correctly the value is tracked through the instructions like mov/copy and subreg manupulation and having only value insn't enough to obtain the reg to store in the DPP instruction.


================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:427
 
   std::vector<MachineInstr*> OrigMIs, DPPMIs;
   if (!OldOpndVGPR.Reg) { // OldOpndVGPR = undef
----------------
nhaehnle wrote:
> These should be SmallVectors.
Ok.


================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:476
       BB->insert(OrigMI, NewMI);
       if (TII->commuteInstruction(*NewMI)) {
         LLVM_DEBUG(dbgs() << "  commuted:  " << *NewMI);
----------------
nhaehnle wrote:
> Can we simplify the code here by just commuting the OrigMI unconditionally?
> 
> That is, first check whether `&Use != src0 && &Use == src1`; in that case, try commuting the instruction. If that fails, break (but I don't think there'd be a need to rollback the commutation). If it succeeds, continue down the same path that is used for the `&Use == src0` case. You end up with only a single call to `createDPPInst` and less code duplication in general.
This is a good idea: I don't like leaving commuted instruction on rollback at some intuitive level but really don't have arguments against it at the moment :-)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55444