[clang] [CIR] Refactor lowerUnion so unions get laid out consistently (PR #210709)

[Erich Keane via cfe-commits]

================
@@ -851,72 +850,61 @@ void CIRRecordLowering::lowerUnion(bool nonVirtualBaseType) {
       fieldType = getStorageType(field);
     }
 
-    // This maps a field to its index. For unions, the index is always 0.
     fieldIdxMap[field->getCanonicalDecl()] = 0;
+    fieldTypes.push_back(fieldType);
+  }
 
-    // Compute zero-initializable status.
-    // This union might not be zero initialized: it may contain a pointer to
-    // data member which might have some exotic initialization sequence.
-    // If this is the case, then we ought not to try and come up with a "better"
-    // type, it might not be very easy to come up with a Constant which
-    // correctly initializes it.
-    if (!seenNamedMember) {
-      seenNamedMember = field->getIdentifier();
-      if (!seenNamedMember)
-        if (const RecordDecl *fieldRD = field->getType()->getAsRecordDecl())
-          seenNamedMember = fieldRD->findFirstNamedDataMember();
-      if (seenNamedMember && !isZeroInitializable(field)) {
-        zeroInitializable = zeroInitializableAsBase = false;
-        storageType = fieldType;
-      }
-    }
+  // Compute zero-initializable status.
+  // This union might not be zero initialized: it may contain a pointer to
+  // data member which might have some exotic initialization sequence.
+  // Unlike classic codegen, we don't really 'give up' on adding all the fields,
+  // though this is a decision/implementation we might want to revisit.  We just
+  // fall back on the typical storage type calculation rather than this bizarre
+  // "choose the first thing", as that likely won't be compatible with later
+  // decisions.
+  for (const FieldDecl *field : recordDecl->fields()) {
 
-    // Because our union isn't zero initializable, we won't be getting a better
-    // storage type.
-    if (!zeroInitializable)
-      continue;
+    auto hasNamedMember = [](const FieldDecl *curField) -> bool {
----------------
erichkeane wrote:

Ah, hmm... I think you're right there.  This DOES result in some stuff no longer being zero-init since I am checking each field.  And yeah, you're right, i don't think I have a test that validates that.  

I actually wonder if the right answer is "Zero-initializable if ANY field is" or "Zero-init if the storage type" is?  WDYT? The latter is a touch unfortunate, since our 'storage type' is determined after the fields are removed, and we previously decided it based on AST zero-init rules. 

Anything we do here is going to be only vaguely related to the previous answers unfortunately.

I'll have to think about this further.

https://github.com/llvm/llvm-project/pull/210709