Skip trivially dead GVs during SROA (#4476)

Similar to how allocas are skipped when they are popped off of the
worklist and found to have no users, GVs are similarly removed and no
further processing is performed. That processing flattened the unused
GV, creating new child GVs and associating the debug info from the
parent with the children. Since the unused GV had all its uses replaced
with another, no debug info is found and the compiler crashed.

Also includes more graceful failure if debug info isn't found for any reason

Add a test that prompts that memcpy replacement order and verifies that
it doesn't crash and doesn't flatten the unused GV

A change was made to the order in which identically sized GVs are processed
in SROA to make the flattened GVs order consistently

Author: Greg Roth

lib/HLSL/HLModule.cpp

@@ -1214,7 +1214,10 @@ void HLModule::CreateElementGlobalVariableDebugInfo(
     unsigned sizeInBits, unsigned alignInBits, unsigned offsetInBits,
     StringRef eltName) {
   DIGlobalVariable *DIGV = dxilutil::FindGlobalVariableDebugInfo(GV, DbgInfoFinder);
-  DXASSERT_NOMSG(DIGV);
+  if (!DIGV) {
+    DXASSERT(DIGV, "DIGV Parameter must be non-null");
+    return;
+  }
   DIBuilder Builder(*GV->getParent());
   DITypeIdentifierMap EmptyMap;
 
@@ -1242,7 +1245,10 @@ void HLModule::UpdateGlobalVariableDebugInfo(
     llvm::GlobalVariable *GV, llvm::DebugInfoFinder &DbgInfoFinder,
     llvm::GlobalVariable *NewGV) {
   DIGlobalVariable *DIGV = dxilutil::FindGlobalVariableDebugInfo(GV, DbgInfoFinder);
-  DXASSERT_NOMSG(DIGV);
+  if (!DIGV) {
+    DXASSERT(DIGV, "DIGV Parameter must be non-null");
+    return;
+  }
   DIBuilder Builder(*GV->getParent());
   DITypeIdentifierMap EmptyMap;
   DIType *DITy = DIGV->getType().resolve(EmptyMap);

lib/Transforms/Scalar/ScalarReplAggregatesHLSL.cpp

@@ -1713,9 +1713,14 @@ bool SROAGlobalAndAllocas(HLModule &HLM, bool bHasDbgInfo) {
         a1ty->isStructTy() && a1ty->getStructNumElements() == 1;
     auto sz0 = DL.getTypeAllocSize(a0ty);
     auto sz1 = DL.getTypeAllocSize(a1ty);
-    if (sz0 == sz1 && (isUnitSzStruct0 || isUnitSzStruct1))
-      return getNestedLevelInStruct(a0ty) < getNestedLevelInStruct(a1ty);
-    return sz0 < sz1;
+    if (sz0 == sz1 && (isUnitSzStruct0 || isUnitSzStruct1)) {
+      sz0 = getNestedLevelInStruct(a0ty);
+      sz1 = getNestedLevelInStruct(a1ty);
+    }
+    // If sizes are equal, tiebreak with alphabetically lesser at higher priority
+    return sz0 < sz1 || (sz0 == sz1 && isa<GlobalVariable>(a0) &&
+                         isa<GlobalVariable>(a1) &&
+                         a0->getName() > a1->getName());
   };
 
   std::priority_queue<Value *, std::vector<Value *>,
@@ -1888,6 +1893,14 @@ bool SROAGlobalAndAllocas(HLModule &HLM, bool bHasDbgInfo) {
       }
     } else {
       GlobalVariable *GV = cast<GlobalVariable>(V);
+      // Handle dead GVs trivially. These can be formed by RAUWing one GV
+      // with another, leaving the original in the worklist
+      if (GV->use_empty()) {
+        GV->eraseFromParent();
+        Changed = true;
+        continue;
+      }
+
       if (staticGVs.count(GV)) {
         Type *Ty = GV->getType()->getPointerElementType();
         // Skip basic types.

tools/clang/test/HLSLFileCheck/passes/hl/sroa_hlsl/memcpy_digv_replacement.hlsl

@@ -0,0 +1,47 @@
+// RUN: %dxc -T vs_6_0 -Zi %s | FileCheck %s
+
+// Test replacement of static global in a case where SROA may try to
+// process a static global replaced by memcpy lowering.
+// This processing includes creating flattened GVs for it
+// and then updating the debug info for them with the original DIGV,
+// which no longer matches because of the replacement, so null is found
+// and unnecessary flattened variables were created.
+// Trivially dead GVs are now removed in SROA
+
+// Ensure the replacement var is flattened
+// CHECK: ReplacementVar.0 = internal unnamed_addr constant [2 x float]
+// CHECK: ReplacementVar.1 = internal unnamed_addr constant [2 x float]
+// CHECK: ReplacementVar.2 = internal unnamed_addr constant [2 x float]
+// CHECK: ReplacementVar.3 = internal unnamed_addr constant [2 x float]
+// Ensure there are no flattened variables created for the replaced GV
+// CHECK-NOT: UnusedVar.[0-3]
+
+// CHECK: @main
+
+// Ensure the replacement var gets its debug info
+// CHECK: DIGlobalVariable(name: "ReplacementVar",
+// CHECK: DIGlobalVariable(name: "ReplacementVar.0",
+// CHECK: DIGlobalVariable(name: "ReplacementVar.1",
+// CHECK: DIGlobalVariable(name: "ReplacementVar.2",
+// CHECK: DIGlobalVariable(name: "ReplacementVar.3",
+
+// Ensure there are no DI for flattened variables created for the replaced GV
+// CHECK-NOT: DIGlobalVariable(name: "UnusedVar.[0-3]",
+
+// All const does is force ReplacementVar to be processed first, which
+// replaces all uses of OrigVar, but leaves OrigVar in the worklist
+// which causes the aforementioned problems when it is reached
+// Otherwise, OrigVar would be encountered first and it would be
+// replaced at the same time as it was retrieved and removed from the worklist
+static const float4 ReplacementVar[2] = { float4(1.0f, 2.0f, 3.0f, 4.0f),
+                                          float4(9.0f, 8.0f, 7.0f, 6.0f) };
+static float4 OrigVar[2];
+
+float4 main(int ix : I) : SV_Position
+{
+  OrigVar = ReplacementVar;
+  return OrigVar[ix];
+}
+
+// Exclude quoted source file (see readme)
+// CHECK-LABEL: {{!"[^"]*\\0A[^"]*"}}