[SelectionDAG] Enhance the simplification of `copyto` from `implicit-def`.

[hliao]

Summary

• The current implementation simplifies the case where the source of copyto is implicit-defed. However, it only works when that implicit-def is single-used since it detects that from implicit-def and cannot determine which destination vreg should be used if there are multiple uses.
• This patch changes that detection when copyto is being emitted. If that copyto's source is defined from implicit-def, it simplifies it. Hence, it works even that implicit-def is multi-used.
• Except it simplifies the internal IR, it won't improve the quality of code generation. However, it helps to detect 'implicit-def` in a straight-forward manner in some passes, such as si-i1-copies. A test case is added.

[hliao]

revise the case, there's no improvement or regression in the existing test.

[nhaehnle]

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

The description also doesn't line up with the test description (which talks about making sure that something doesn't crash).

[hliao]

In D62342#1515284, @nhaehnle wrote:

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

The description also doesn't line up with the test description (which talks about making sure that something doesn't crash).

IMPLICIT_DEF is multi-instantiated in lowering, i.e. each use of it has a dedicated definition of IMPLICIT_DEF (refer to getVR and getDstOfOnlyCopyToRegUse). The only slight simplification is for the sequence of COPY_TO followed by IMPLICIT_DEF on virtual register dest. In that case, that COPY_TO could be eliminated. IMPLICIT_DEF is just undef, the implementation has the right to decide how to do it efficiently.

[arsenm]

In D62342#1515284, @nhaehnle wrote:

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

I don't think this is a guaranteed property. This is the MIR equivalent of the poison/freeze question, but today implicit_def doesn't give you this

[arsenm]

llvm/test/CodeGen/AMDGPU/i1-copy-phi.ll

41–42 ↗

(On Diff #201125)

This should check something. I also suspect it can be reduced further

[hliao]

reduce test further

[hliao]

test is reduced further

[hliao]

reduce even further

[hliao]

llvm/test/CodeGen/AMDGPU/i1-copy-phi.ll

41–42 ↗

(On Diff #201125)

test is reduced

[nhaehnle]

In D62342#1515727, @arsenm wrote:

In D62342#1515284, @nhaehnle wrote:

I don't think this is correct. If an IMPLICIT_DEF has multiple uses, then the correctness of the program may depend on all uses seeing the same value, even if that value is undefined.

I don't think this is a guaranteed property. This is the MIR equivalent of the poison/freeze question, but today implicit_def doesn't give you this

Hmm alright. Then the change itself seems okay.

llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.h

class LLVM_LIBRARY_VISIBILITY InstrEmitter {
 
   /// EmitCopyFromReg - Generate machine code for an CopyFromReg node or an
   /// implicit physical register output.
   void EmitCopyFromReg(SDNode *Node, unsigned ResNo,
                        bool IsClone, bool IsCloned,
                        unsigned SrcReg,
                        DenseMap<SDValue, unsigned> &VRBaseMap);
 
-  /// getDstOfCopyToRegUse - If the only use of the specified result number of
-  /// node is a CopyToReg, return its destination register. Return 0 otherwise.
-  unsigned getDstOfOnlyCopyToRegUse(SDNode *Node,
-                                    unsigned ResNo) const;
-
   void CreateVirtualRegisters(SDNode *Node,
                               MachineInstrBuilder &MIB,
                               const MCInstrDesc &II,
                               bool IsClone, bool IsCloned,
                               DenseMap<SDValue, unsigned> &VRBaseMap);
 
   /// getVR - Return the virtual register corresponding to the specified result
   /// of the specified node.

llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

EmitCopyFromReg(SDNode *Node, unsigned ResNo, bool IsClone, bool IsCloned,
   SDValue Op(Node, ResNo);
   if (IsClone)
     VRBaseMap.erase(Op);
   bool isNew = VRBaseMap.insert(std::make_pair(Op, VRBase)).second;
   (void)isNew; // Silence compiler warning.
   assert(isNew && "Node emitted out of order - early");
 }
 
-/// getDstOfCopyToRegUse - If the only use of the specified result number of
-/// node is a CopyToReg, return its destination register. Return 0 otherwise.
-unsigned InstrEmitter::getDstOfOnlyCopyToRegUse(SDNode *Node,
-                                                unsigned ResNo) const {
-  if (!Node->hasOneUse())
-    return 0;
-
-  SDNode *User = *Node->use_begin();
-  if (User->getOpcode() == ISD::CopyToReg &&
-      User->getOperand(2).getNode() == Node &&
-      User->getOperand(2).getResNo() == ResNo) {
-    unsigned Reg = cast<RegisterSDNode>(User->getOperand(1))->getReg();
-    if (TargetRegisterInfo::isVirtualRegister(Reg))
-      return Reg;
-  }
-  return 0;
-}
-
 void InstrEmitter::CreateVirtualRegisters(SDNode *Node,
                                        MachineInstrBuilder &MIB,
                                        const MCInstrDesc &II,
                                        bool IsClone, bool IsCloned,
                                        DenseMap<SDValue, unsigned> &VRBaseMap) {
   assert(Node->getMachineOpcode() != TargetOpcode::IMPLICIT_DEF &&
          "IMPLICIT_DEF should have been handled as a special case elsewhere!");
 

 
 /// getVR - Return the virtual register corresponding to the specified result
 /// of the specified node.
 unsigned InstrEmitter::getVR(SDValue Op,
                              DenseMap<SDValue, unsigned> &VRBaseMap) {
   if (Op.isMachineOpcode() &&
       Op.getMachineOpcode() == TargetOpcode::IMPLICIT_DEF) {
     // Add an IMPLICIT_DEF instruction before every use.
-    unsigned VReg = getDstOfOnlyCopyToRegUse(Op.getNode(), Op.getResNo());
     // IMPLICIT_DEF can produce any type of result so its MCInstrDesc
     // does not include operand register class info.
-    if (!VReg) {
-      const TargetRegisterClass *RC = TLI->getRegClassFor(
-          Op.getSimpleValueType(), Op.getNode()->isDivergent());
-      VReg = MRI->createVirtualRegister(RC);
-    }
+    unsigned VReg = MRI->createVirtualRegister(RC);
     BuildMI(*MBB, InsertPos, Op.getDebugLoc(),
             TII->get(TargetOpcode::IMPLICIT_DEF), VReg);
     return VReg;
   }
 
   DenseMap<SDValue, unsigned>::iterator I = VRBaseMap.find(Op);
   assert(I != VRBaseMap.end() && "Node emitted out of order - late");
   return I->second;

 #endif
     llvm_unreachable("This target-independent node should have been selected!");
   case ISD::EntryToken:
     llvm_unreachable("EntryToken should have been excluded from the schedule!");
   case ISD::MERGE_VALUES:
   case ISD::TokenFactor: // fall thru
     break;
   case ISD::CopyToReg: {
-    unsigned SrcReg;
+    unsigned DestReg = cast<RegisterSDNode>(Node->getOperand(1))->getReg();
     SDValue SrcVal = Node->getOperand(2);
+    if (TargetRegisterInfo::isVirtualRegister(DestReg) &&
+        SrcVal.isMachineOpcode() &&
+        SrcVal.getMachineOpcode() == TargetOpcode::IMPLICIT_DEF) {
+      // Instead building a COPY to that vreg destination, build an
+      // IMPLICIT_DEF instruction instead.
+      BuildMI(*MBB, InsertPos, Node->getDebugLoc(),
+              TII->get(TargetOpcode::IMPLICIT_DEF), DestReg);
+      break;
+    }
+    unsigned SrcReg;
     if (RegisterSDNode *R = dyn_cast<RegisterSDNode>(SrcVal))
       SrcReg = R->getReg();
     else
       SrcReg = getVR(SrcVal, VRBaseMap);
 
-    unsigned DestReg = cast<RegisterSDNode>(Node->getOperand(1))->getReg();
     if (SrcReg == DestReg) // Coalesced away the copy? Ignore.
       break;
 
     BuildMI(*MBB, InsertPos, Node->getDebugLoc(), TII->get(TargetOpcode::COPY),
             DestReg).addReg(SrcReg);
     break;
   }
   case ISD::CopyFromReg: {

llvm/trunk/test/CodeGen/AMDGPU/i1-copy-phi.ll

 
 bb6:                                              ; preds = %bb4, %bb3
   ret void
 }
 
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 
 attributes #0 = { nounwind readnone }
+
+; Make sure this won't crash.
+; SI-LABEL: {{^}}vcopy_i1_undef
+; SI: v_cndmask_b32_e64
+; SI: v_cndmask_b32_e64
+define <2 x float> @vcopy_i1_undef(<2 x float> addrspace(1)* %p) {
+entry:
+  br i1 undef, label %exit, label %false
+
+false:
+  %x = load <2 x float>, <2 x float> addrspace(1)* %p
+  %cmp = fcmp one <2 x float> %x, zeroinitializer
+  br label %exit
+
+exit:
+  %c = phi <2 x i1> [ undef, %entry ], [ %cmp, %false ]
+  %ret = select <2 x i1> %c, <2 x float> <float 2.0, float 2.0>, <2 x float> <float 4.0, float 4.0>
+  ret <2 x float> %ret
+}

llvm/trunk/test/CodeGen/AMDGPU/implicit-def-muse.ll

+; RUN: llc -march=amdgcn -stop-after=amdgpu-isel -verify-machineinstrs -o - %s | FileCheck %s
+
+; CHECK-LABEL: vcopy_i1_undef
+; CHECK: IMPLICIT_DEF
+; CHECK-NOT: COPY
+; CHECK: IMPLICIT_DEF
+; CHECK-NOT: COPY
+; CHECK: .false:
+define <2 x float> @vcopy_i1_undef(<2 x float> addrspace(1)* %p) {
+entry:
+  br i1 undef, label %exit, label %false
+
+false:
+  %x = load <2 x float>, <2 x float> addrspace(1)* %p
+  %cmp = fcmp one <2 x float> %x, zeroinitializer
+  br label %exit
+
+exit:
+  %c = phi <2 x i1> [ undef, %entry ], [ %cmp, %false ]
+  %ret = select <2 x i1> %c, <2 x float> <float 2.0, float 2.0>, <2 x float> <float 4.0, float 4.0>
+  ret <2 x float> %ret
+}