[PATCH] D150390: [AMDGPU] Introduce and use the new PRED_COPY opcode

[Yashwant Singh via Phabricator via llvm-commits]

yassingh marked 6 inline comments as done.
yassingh added a comment.

In D150390#4349478 <https://reviews.llvm.org/D150390#4349478>, @Pierre-vh wrote:

> I haven't followed this closely, but how will this interact with GlobalISel?

No. It's introduced in the greedy register allocator and removed right after.



================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:3372-3374
+  // PRED_COPY is anyway lowered to COPY so this flag is not necessary
+  // but it messes up with MI.getNumOperands() not returning 2
+  // let Uses = [EXEC];
----------------
cdevadas wrote:
> Pierre-vh wrote:
> > Do you have more detail on the failure that's happening when this is used?
> > Out of correctness it might be nice to have the `implicit $exec`
> > 
> > (note: no strong opinion either so if other reviewers think it's fine as-is, then it's good for me too)
> The `SIFixVGPRCopies` pass inserted post regalloc pipeline will add the implicit exec for all vector copies. The Pred_COPY opcode is only short-lived. It will only get introduced by the liverange-split during regalloc and later `SISimplifyPredicatedCopies` inserted before `virtregrewriter` changes the Pred_COPY back to COPY opcode after manipulating the wwm-copy as needed.
> It's ok even if we don't add the implicit use here.
> 
> 
> 
> 
```
static const TargetRegisterClass *canFoldCopy(const MachineInstr &MI,
                                              unsigned FoldIdx) {
  const TargetInstrInfo &TII = *MI.getParent()->getParent()->getSubtarget().getInstrInfo();
  assert(TII.isCopyInstr(MI) && "MI must be a COPY instruction");
  if (MI.getNumOperands() != 2)
    return nullptr;
```
This function will early exit if we have 'implicit exec' right from the start for PRED_COPY. There might be more places where we will run into similar problems(haven't fully explored them).

Since we lower PRED_COPY back to COPY which explicitly adds 'exec' operand in SIFixVGPRCopies not having this flag here doesn't affect anything.


================
Comment at: llvm/lib/Target/AMDGPU/SISimplifyPredicatedCopies.cpp:79-81
+        // Below asserts still fails cause isVgprCopy call isCopy() which is
+        // false for PRED_COPY. Figure out a way to use isCopyInstr()
+        // everywhere. assert(TII->isVGPRCopy(MI));
----------------
I accidentally added this comment here, removed in dependent patch D143762.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150390