[PATCH] D63731: [AMDGPU] Prevent VGPR copies from moving across the EXEC mask definitions

[Alexander via Phabricator via llvm-commits]

alex-t marked 2 inline comments as done.
alex-t added inline comments.


================
Comment at: lib/Target/AMDGPU/SIFoldOperands.cpp:504
+    MachineInstr::mop_iterator ImpOpE = UseMI->implicit_operands().end();
+    while (ImpOpI != ImpOpE) {
+      MachineInstr::mop_iterator Tmp = ImpOpI;
----------------
rampitec wrote:
> I believe the intent was to remove only duplicate operands, not all implicit operands at all.
For the AMDGPU::COPY, that is the only case here, we don't expect any implicit operands to exist except those added by legalizeGenericOperand.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3923-3930
+  bool ImpDef = Def->isImplicitDef();
+  while (!ImpDef && Def && Def->isCopy()) {
+    Def = MRI.getUniqueVRegDef(Def->getOperand(1).getReg());
+    ImpDef = Def && Def->isImplicitDef();
+  }
+  if (!RI.isSGPRClass(DstRC) && !Copy->readsRegister(AMDGPU::EXEC, &RI) &&
+      !ImpDef)
----------------
nhaehnle wrote:
> alex-t wrote:
> > arsenm wrote:
> > > I don't see why this is necessary 
> > To prevent moving VGPR copies across the EXEC definitions
> I think the question is more, why isn't the addOperand executed unconditionally for non-SGPR destinations? That is:
> ```
>   if (!RI.isSGPRClass(DstRC) && !Copy->readsRegister(AMDGPU::EXEC, &RI))
>     Copy->addOperand(MachineOperand::CreateReg(AMDGPU::EXEC, false, true));
> ```
> ... without the preceding loop.
> 
> Presumably the answer might have to do with PHIs that have undef / IMPLICIT_DEF operands? Though, isn't it preferable for those to just be reduced to an IMPLICIT_DEF instead of a COPY from an IMPLICIT_DEF? And if the COPY is optimized away later, does adding the EXEC really hurt?
The PHIs that have an input of COPY from IMPLICIT_DEF were exactly the reason for the "preceding loop" :)
Since the COPY from IMPLICIT_DEF to VGPR gets equipped with implicit use of EXEC it is not considered as the conversion candidate by the processImplicitDefs pass. So, I just excluded them from marking as using EXEC.


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

https://reviews.llvm.org/D63731