[PATCH] D55444: AMDGPU: Fix DPP combiner

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Hi Valery, I really like the way the different cases are listed in the explanatory comment at the top of the file, and I believe those cases are correct. Would it be possible to restructure the code in a way that follows those cases? I think that would make it much easier to follow.

That is, in `combineDPPMov`, can you restructure the first do/while construct so that it mirrors the cases of the top of file comment and defines new variables `CombinedOld` and `CombinedBoundCtrl`? Come to think of it, it may then be wise to move these top of file comments into the function to increase the chances that they will be kept up-to-date in the future.



================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:358
   assert(OldOpnd && OldOpnd->isReg());
   auto OldOpndVGPR = getRegSubRegPair(*OldOpnd);
   auto *OldOpndValue = getOldOpndValue(*OldOpnd);
----------------
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.


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


================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:476
       BB->insert(OrigMI, NewMI);
       if (TII->commuteInstruction(*NewMI)) {
         LLVM_DEBUG(dbgs() << "  commuted:  " << *NewMI);
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55444