[llvm] r371508 - [AMDGPU]: PHI Elimination hooks added for custom COPY insertion.

Timofeev, Alexander via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 13 09:11:31 PDT 2019


Hi Galina,

I’m now testing the fix for that issue.
Given that it’s Friday evening what would you recommend?
Open code review for the fix and mark test as XFAIL?
Revert the whole patch and wait until Monday?

From: Galina Kistanova [mailto:gkistanova at gmail.com]
Sent: Thursday, September 12, 2019 9:13 PM
To: Timofeev, Alexander
Cc: llvm-commits
Subject: Re: [llvm] r371508 - [AMDGPU]: PHI Elimination hooks added for custom COPY insertion.

[CAUTION: External Email]
Please have a look ASAP?
Thanks

Galina

On Wed, Sep 11, 2019 at 1:40 PM Galina Kistanova <gkistanova at gmail.com<mailto:gkistanova at gmail.com>> wrote:
Hello Alexander,

This commit added broken test to the builder llvm-clang-x86_64-expensive-checks-win.
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19594:
. . .
Failing Tests (1):
    LLVM :: CodeGen/AMDGPU/phi-elimination-end-cf.mir

The builder was already red and did not send notifications on this.
Please have a look?

Thanks

Galina

On Tue, Sep 10, 2019 at 3:57 AM Alexander Timofeev via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
Author: alex-t
Date: Tue Sep 10 03:58:57 2019
New Revision: 371508

URL: http://llvm.org/viewvc/llvm-project?rev=371508&view=rev
Log:
[AMDGPU]: PHI Elimination hooks added for custom COPY insertion.

  Reviewers: rampitec, vpykhtin

  Differential Revision: https://reviews.llvm.org/D67101

Added:
    llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-end-cf.mir
Modified:
    llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h
    llvm/trunk/lib/CodeGen/PHIElimination.cpp
    llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp
    llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h
    llvm/trunk/lib/Target/AMDGPU/SILowerControlFlow.cpp
    llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-assertion.mir

Modified: llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h?rev=371508&r1=371507&r2=371508&view=diff
==============================================================================
--- llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h (original)
+++ llvm/trunk/include/llvm/CodeGen/TargetInstrInfo.h Tue Sep 10 03:58:57 2019
@@ -22,6 +22,7 @@
 #include "llvm/CodeGen/MachineCombinerPattern.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
+#include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineOutliner.h"
@@ -1638,6 +1639,28 @@ public:
     return false;
   }

+  /// During PHI eleimination lets target to make necessary checks and
+  /// insert the copy to the PHI destination register in a target specific
+  /// manner.
+  virtual MachineInstr *createPHIDestinationCopy(
+      MachineBasicBlock &MBB, MachineBasicBlock::iterator InsPt,
+      const DebugLoc &DL, Register Src, Register Dst) const {
+    return BuildMI(MBB, InsPt, DL, get(TargetOpcode::COPY), Dst)
+        .addReg(Src);
+  }
+
+  /// During PHI eleimination lets target to make necessary checks and
+  /// insert the copy to the PHI destination register in a target specific
+  /// manner.
+  virtual MachineInstr *createPHISourceCopy(MachineBasicBlock &MBB,
+                                            MachineBasicBlock::iterator InsPt,
+                                            const DebugLoc &DL, Register Src,
+                                            Register SrcSubReg,
+                                            Register Dst) const {
+    return BuildMI(MBB, InsPt, DL, get(TargetOpcode::COPY), Dst)
+        .addReg(Src, 0, SrcSubReg);
+  }
+
   /// Returns a \p outliner::OutlinedFunction struct containing target-specific
   /// information for a set of outlining candidates.
   virtual outliner::OutlinedFunction getOutliningCandidateInfo(

Modified: llvm/trunk/lib/CodeGen/PHIElimination.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/PHIElimination.cpp?rev=371508&r1=371507&r2=371508&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/PHIElimination.cpp (original)
+++ llvm/trunk/lib/CodeGen/PHIElimination.cpp Tue Sep 10 03:58:57 2019
@@ -31,7 +31,9 @@
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
+#include "llvm/CodeGen/TargetLowering.h"
 #include "llvm/CodeGen/TargetOpcodes.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/Pass.h"
@@ -252,11 +254,12 @@ void PHIElimination::LowerPHINode(Machin
   // Insert a register to register copy at the top of the current block (but
   // after any remaining phi nodes) which copies the new incoming register
   // into the phi node destination.
+  MachineInstr *PHICopy = nullptr;
   const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
   if (allPhiOperandsUndefined(*MPhi, *MRI))
     // If all sources of a PHI node are implicit_def or undef uses, just emit an
     // implicit_def instead of a copy.
-    BuildMI(MBB, AfterPHIsIt, MPhi->getDebugLoc(),
+    PHICopy = BuildMI(MBB, AfterPHIsIt, MPhi->getDebugLoc(),
             TII->get(TargetOpcode::IMPLICIT_DEF), DestReg);
   else {
     // Can we reuse an earlier PHI node? This only happens for critical edges,
@@ -273,15 +276,13 @@ void PHIElimination::LowerPHINode(Machin
       const TargetRegisterClass *RC = MF.getRegInfo().getRegClass(DestReg);
       entry = IncomingReg = MF.getRegInfo().createVirtualRegister(RC);
     }
-    BuildMI(MBB, AfterPHIsIt, MPhi->getDebugLoc(),
-            TII->get(TargetOpcode::COPY), DestReg)
-      .addReg(IncomingReg);
+    // Give the target possiblity to handle special cases fallthrough otherwise
+    PHICopy = TII->createPHIDestinationCopy(MBB, AfterPHIsIt, MPhi->getDebugLoc(),
+                                  IncomingReg, DestReg);
   }

   // Update live variable information if there is any.
   if (LV) {
-    MachineInstr &PHICopy = *std::prev(AfterPHIsIt);
-
     if (IncomingReg) {
       LiveVariables::VarInfo &VI = LV->getVarInfo(IncomingReg);

@@ -302,7 +303,7 @@ void PHIElimination::LowerPHINode(Machin
       // killed.  Note that because the value is defined in several places (once
       // each for each incoming block), the "def" block and instruction fields
       // for the VarInfo is not filled in.
-      LV->addVirtualRegisterKilled(IncomingReg, PHICopy);
+      LV->addVirtualRegisterKilled(IncomingReg, *PHICopy);
     }

     // Since we are going to be deleting the PHI node, if it is the last use of
@@ -312,15 +313,14 @@ void PHIElimination::LowerPHINode(Machin

     // If the result is dead, update LV.
     if (isDead) {
-      LV->addVirtualRegisterDead(DestReg, PHICopy);
+      LV->addVirtualRegisterDead(DestReg, *PHICopy);
       LV->removeVirtualRegisterDead(DestReg, *MPhi);
     }
   }

   // Update LiveIntervals for the new copy or implicit def.
   if (LIS) {
-    SlotIndex DestCopyIndex =
-        LIS->InsertMachineInstrInMaps(*std::prev(AfterPHIsIt));
+    SlotIndex DestCopyIndex = LIS->InsertMachineInstrInMaps(*PHICopy);

     SlotIndex MBBStartIndex = LIS->getMBBStartIdx(&MBB);
     if (IncomingReg) {
@@ -406,9 +406,9 @@ void PHIElimination::LowerPHINode(Machin
           if (DefMI->isImplicitDef())
             ImpDefs.insert(DefMI);
       } else {
-        NewSrcInstr = BuildMI(opBlock, InsertPos, MPhi->getDebugLoc(),
-                            TII->get(TargetOpcode::COPY), IncomingReg)
-                        .addReg(SrcReg, 0, SrcSubReg);
+        NewSrcInstr =
+            TII->createPHISourceCopy(opBlock, InsertPos, MPhi->getDebugLoc(),
+                                     SrcReg, SrcSubReg, IncomingReg);
       }
     }

@@ -457,7 +457,7 @@ void PHIElimination::LowerPHINode(Machin
           }
         } else {
           // We just inserted this copy.
-          KillInst = std::prev(InsertPos);
+          KillInst = NewSrcInstr;
         }
       }
       assert(KillInst->readsRegister(SrcReg) && "Cannot find kill instruction");

Modified: llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp?rev=371508&r1=371507&r2=371508&view=diff
==============================================================================
--- llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp (original)
+++ llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp Tue Sep 10 03:58:57 2019
@@ -6410,3 +6410,31 @@ bool llvm::execMayBeModifiedBeforeAnyUse
       return true;
   }
 }
+
+MachineInstr *SIInstrInfo::createPHIDestinationCopy(
+    MachineBasicBlock &MBB, MachineBasicBlock::iterator LastPHIIt,
+    const DebugLoc &DL, Register Src, Register Dst) const {
+  auto Cur = MBB.begin();
+  do {
+    if (!Cur->isPHI() && Cur->readsRegister(Dst))
+      return BuildMI(MBB, Cur, DL, get(TargetOpcode::COPY), Dst).addReg(Src);
+    ++Cur;
+  } while (Cur != MBB.end() && Cur != LastPHIIt);
+
+  return TargetInstrInfo::createPHIDestinationCopy(MBB, LastPHIIt, DL, Src,
+                                                   Dst);
+}
+
+MachineInstr *SIInstrInfo::createPHISourceCopy(
+    MachineBasicBlock &MBB, MachineBasicBlock::iterator InsPt,
+    const DebugLoc &DL, Register Src, Register SrcSubReg, Register Dst) const {
+  if (InsPt != MBB.end() && InsPt->isPseudo() && InsPt->definesRegister(Src)) {
+    InsPt++;
+    return BuildMI(MBB, InsPt, InsPt->getDebugLoc(), get(TargetOpcode::COPY),
+                   Dst)
+        .addReg(Src, 0, SrcSubReg)
+        .addReg(AMDGPU::EXEC, RegState::Implicit);
+  }
+  return TargetInstrInfo::createPHISourceCopy(MBB, InsPt, DL, Src, SrcSubReg,
+                                              Dst);
+}

Modified: llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h?rev=371508&r1=371507&r2=371508&view=diff
==============================================================================
--- llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h (original)
+++ llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h Tue Sep 10 03:58:57 2019
@@ -954,6 +954,17 @@ public:

   bool isBasicBlockPrologue(const MachineInstr &MI) const override;

+  MachineInstr *createPHIDestinationCopy(MachineBasicBlock &MBB,
+                                         MachineBasicBlock::iterator InsPt,
+                                         const DebugLoc &DL, Register Src,
+                                         Register Dst) const override;
+
+  MachineInstr *createPHISourceCopy(MachineBasicBlock &MBB,
+                                    MachineBasicBlock::iterator InsPt,
+                                    const DebugLoc &DL, Register Src,
+                                    Register SrcSubReg,
+                                    Register Dst) const override;
+
   /// Return a partially built integer add instruction without carry.
   /// Caller must add source operands.
   /// For pre-GFX9 it will generate unused carry destination operand.

Modified: llvm/trunk/lib/Target/AMDGPU/SILowerControlFlow.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/SILowerControlFlow.cpp?rev=371508&r1=371507&r2=371508&view=diff
==============================================================================
--- llvm/trunk/lib/Target/AMDGPU/SILowerControlFlow.cpp (original)
+++ llvm/trunk/lib/Target/AMDGPU/SILowerControlFlow.cpp Tue Sep 10 03:58:57 2019
@@ -400,13 +400,17 @@ void SILowerControlFlow::emitLoop(Machin

 void SILowerControlFlow::emitEndCf(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
+  MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
+  unsigned CFMask = MI.getOperand(0).getReg();
+  MachineInstr *Def = MRI.getUniqueVRegDef(CFMask);
   const DebugLoc &DL = MI.getDebugLoc();

-  MachineBasicBlock::iterator InsPt = MBB.begin();
-  MachineInstr *NewMI =
-      BuildMI(MBB, InsPt, DL, TII->get(OrOpc), Exec)
-          .addReg(Exec)
-          .add(MI.getOperand(0));
+  MachineBasicBlock::iterator InsPt =
+      Def && Def->getParent() == &MBB ? std::next(MachineBasicBlock::iterator(Def))
+                               : MBB.begin();
+  MachineInstr *NewMI = BuildMI(MBB, InsPt, DL, TII->get(OrOpc), Exec)
+                            .addReg(Exec)
+                            .add(MI.getOperand(0));

   if (LIS)
     LIS->ReplaceMachineInstrInMaps(MI, *NewMI);

Modified: llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-assertion.mir
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-assertion.mir?rev=371508&r1=371507&r2=371508&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-assertion.mir (original)
+++ llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-assertion.mir Tue Sep 10 03:58:57 2019
@@ -26,8 +26,8 @@ body:             |

 # CHECK-LABEL: name:            foo
 # CHECK:   bb.3:
-# CHECK-NEXT:     %3:sreg_32_xm0 = COPY killed %4
 # CHECK-NEXT:     dead %2:sreg_32_xm0 = IMPLICIT_DEF
+# CHECK-NEXT:     %3:sreg_32_xm0 = COPY killed %4
 # CHECK-NEXT:     S_NOP 0, implicit killed %3



Added: llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-end-cf.mir
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-end-cf.mir?rev=371508&view=auto
==============================================================================
--- llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-end-cf.mir (added)
+++ llvm/trunk/test/CodeGen/AMDGPU/phi-elimination-end-cf.mir Tue Sep 10 03:58:57 2019
@@ -0,0 +1,54 @@
+# RUN: llc -mtriple amdgcn -run-pass livevars -run-pass phi-node-elimination -o - %s | FileCheck %s
+
+# CHECK-LABEL:  phi-cf-test
+# CHECK: bb.0:
+# CHECK:     [[COND:%[0-9]+]]:sreg_64 = V_CMP_EQ_U32_e64
+# CHECK:     [[IF_SOURCE0:%[0-9]+]]:sreg_64 = SI_IF [[COND]], %bb.1, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+# CHECK:     [[IF_INPUT_REG:%[0-9]+]]:sreg_64 = COPY killed [[IF_SOURCE0]], implicit $exec
+
+# CHECK: bb.1:
+# CHECK:     [[END_CF_ARG:%[0-9]+]]:sreg_64 = COPY killed [[IF_INPUT_REG]]
+# CHECK:     SI_END_CF killed [[END_CF_ARG]], implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+
+# CHECK: bb.2:
+# CHECK:     [[IF_SOURCE1:%[0-9]+]]:sreg_64 = SI_IF [[COND]], %bb.1, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+# CHECK:     [[IF_INPUT_REG]]:sreg_64 = COPY killed [[IF_SOURCE1]], implicit $exec
+
+
+...
+---
+name:            phi-cf-test
+tracksRegLiveness: true
+body:             |
+
+  bb.0:
+    successors: %bb.3(0x40000000), %bb.2(0x40000000)
+    liveins: $vgpr0
+
+    %5:vgpr_32(s32) = COPY $vgpr0
+    %0:sreg_64 = V_CMP_EQ_U32_e64 0, %5(s32), implicit $exec
+    %18:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    %22:sreg_64 = SI_IF %0, %bb.2, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+    S_BRANCH %bb.3
+
+  bb.2:
+    successors: %bb.3(0x80000000)
+
+    %24:sreg_64 = PHI %20, %bb.3, %22, %bb.0
+    %23:vgpr_32 = PHI %19, %bb.3, %18, %bb.0
+    SI_END_CF %24, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+    %3:vgpr_32, dead %10:sreg_64 = nsw V_ADD_I32_e64 1, %23, 0, implicit $exec
+
+  bb.3:
+    successors: %bb.3(0x40000000), %bb.2(0x40000000)
+
+    %4:vgpr_32 = PHI %19, %bb.3, %3, %bb.2, %18, %bb.0
+    %15:sreg_32_xm0 = S_MOV_B32 61440
+    %16:sreg_32_xm0 = S_MOV_B32 -1
+    %17:sreg_128 = REG_SEQUENCE undef %14:sreg_32_xm0, %subreg.sub0, undef %12:sreg_32_xm0, %subreg.sub1, %16, %subreg.sub2, %15, %subreg.sub3
+    BUFFER_STORE_DWORD_OFFSET %4, %17, 0, 0, 0, 0, 0, 0, implicit $exec :: (volatile store 4 into `i32 addrspace(1)* undef`, addrspace 1)
+    %19:vgpr_32 = COPY %4
+    %20:sreg_64 = SI_IF %0, %bb.2, implicit-def dead $exec, implicit-def dead $scc, implicit $exec
+    S_BRANCH %bb.3
+
+...


_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190913/edb2c265/attachment.html>


More information about the llvm-commits mailing list