[CodeGen] Always update divergence in SelectionDAG::UpdateNodeOperands

[scott.linder]

Summary

Only one overload of SelectionDAG::UpdateNodeOperands was modified to update divergence information in https://reviews.llvm.org/D35267 which leads to IsDivergent bits becoming stale, and so verification passes failing with an assert.

The test case is as simple as I could manage while still triggering the bug; redundant instructions are needed to get the correct combining/legalizing to call the right overloads.

[arsenm]

test/CodeGen/AMDGPU/vector-legalizer-divergence.ll

1

Needs to check something

[scott.linder]

test/CodeGen/AMDGPU/vector-legalizer-divergence.ll

1

I am not sure what to check; currently it just triggers the assert, so it is a noop in non-assert builds.

The bug does not show up in the divergence analysis itself, so opt -analyze -divergence is not enough.

[scott.linder]

Ping

I am still not certain how to improve the test, but I think this is a fairly straightforward patch

[alex-t]

My apologies for the delay. Thanks for handling this. LGTM.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDNode *SelectionDAG::UpdateNodeOperands(SDNode *N, SDValue Op) {
   // Nope it doesn't.  Remove the node from its current place in the maps.
   if (InsertPos)
     if (!RemoveNodeFromCSEMaps(N))
       InsertPos = nullptr;
 
   // Now we update the operands.
   N->OperandList[0].set(Op);
 
+  updateDivergence(N);
   // If this gets put into a CSE map, add it.
   if (InsertPos) CSEMap.InsertNode(N, InsertPos);
   return N;
 }
 
 SDNode *SelectionDAG::UpdateNodeOperands(SDNode *N, SDValue Op1, SDValue Op2) {
   assert(N->getNumOperands() == 2 && "Update with wrong number of operands");
 
if (InsertPos)
     if (!RemoveNodeFromCSEMaps(N))
       InsertPos = nullptr;
 
   // Now we update the operands.
   for (unsigned i = 0; i != NumOps; ++i)
     if (N->OperandList[i] != Ops[i])
       N->OperandList[i].set(Ops[i]);
 
+  updateDivergence(N);
   // If this gets put into a CSE map, add it.
   if (InsertPos) CSEMap.InsertNode(N, InsertPos);
   return N;
 }
 
 /// DropOperands - Release the operands and set this node to have
 /// zero operands.
 void SDNode::DropOperands() {

test/CodeGen/AMDGPU/vector-legalizer-divergence.ll

+; RUN: llc -march=amdgcn < %s
+
+; Tests for a bug in SelectionDAG::UpdateNodeOperands exposed by VectorLegalizer
+; where divergence information is not updated.
+
+declare i32 @llvm.amdgcn.workitem.id.x()
+
+define amdgpu_kernel void @spam(double addrspace(1)* noalias %arg) {
+  %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
+  %tmp1 = zext i32 %tmp to i64
+  %tmp2 = getelementptr inbounds double, double addrspace(1)* %arg, i64 %tmp1
+  %tmp3 = load double, double addrspace(1)* %tmp2, align 8
+  %tmp4 = fadd double undef, 0.000000e+00
+  %tmp5 = insertelement <2 x double> undef, double %tmp4, i64 0
+  %tmp6 = insertelement <2 x double> %tmp5, double %tmp3, i64 1
+  %tmp7 = insertelement <2 x double> %tmp6, double 0.000000e+00, i64 1
+  %tmp8 = fadd <2 x double> zeroinitializer, undef
+  %tmp9 = fadd <2 x double> %tmp7, zeroinitializer
+  %tmp10 = extractelement <2 x double> %tmp8, i64 0
+  %tmp11 = getelementptr inbounds double, double addrspace(1)* %tmp2, i64 2
+  store double %tmp10, double addrspace(1)* %tmp11, align 8
+  %tmp12 = getelementptr inbounds double, double addrspace(1)* %tmp2, i64 3
+  store double undef, double addrspace(1)* %tmp12, align 8
+  %tmp13 = extractelement <2 x double> %tmp9, i64 0
+  %tmp14 = getelementptr inbounds double, double addrspace(1)* %tmp2, i64 6
+  store double %tmp13, double addrspace(1)* %tmp14, align 8
+  %tmp15 = getelementptr inbounds double, double addrspace(1)* %tmp2, i64 7
+  store double 0.000000e+00, double addrspace(1)* %tmp15, align 8
+  ret void
+}