[PATCH] D133283: [DirectX backend] Support global ctor for DXILBitcodeWriter.

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

As an FYI (not something I expect you to tackle here), I just filed an issue to track writing an analysis printer for the pointer analysis pass (https://github.com/llvm/llvm-project/issues/57934). That would really help us debug and test this pass as it grows in complexity.

I have some other comments below mostly nitpicks about comment wording, but a few questions too. Overall looking good.



================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:353
+  // Function/GlobalVariable in the map.
+  unsigned getGlobalObjectValueTypeID(Type *T, const GlobalObject *G);
 };
----------------
I assume this is true for all `llvm::GlobalObject`s right?

Maybe:
```
  /// getGlobalObjectValueTypeID - returns the element type for a GlobalObject
  ///
  /// GlobalObject types are saved by PointerTypeAnalysis as pointers to the 
  /// GlobalObject, but in the bitcode writer we need the pointer element type.
```


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:561
     return VE.getTypeID(It->second);
+  if (!T->isOpaquePointerTy())
+    return VE.getTypeID(T);
----------------
The point of this being higher up in the function was to early out and avoid the cost of the map lookup. With your new code, is there a way we can rewrite this check instead of moving it?


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.cpp:1220
+    // Here use getGlobalObjectValueTypeID for the Abbrev which store ValueType
+    // of GlobalVariable.
+    MaxGlobalType = std::max(
----------------
You’re not actually looking up the abbreviation, but rather the enumerated type identifier.

Maybe something more like:
```
// Use getGlobalObjectValueTypeID to look up the enumerated type ID for Global Variable types.
``` 


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:111
+  // FIXME: support ConstantPointerNull which could map to more than one
+  // TypedPointerType.
+  auto It = Map.find(C);
----------------
Issue please.


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:116
+  // Skip ConstantData which cannot have opaque ptr.
+  if (isa<ConstantData>(C))
+    return C->getType();
----------------
Can we move this up above the map lookup?


================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:147
+  // Must have a target ty when map.
+  assert(TargetTy);
+
----------------



================
Comment at: llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp:188
   }
-
+  // classify special GVs last to make sure functions are classified.
+  for (auto *G : SpecialGVs) {
----------------
I understand needing functions classified first, but this will break if we ever have any other global variable that contains function pointers right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133283/new/

https://reviews.llvm.org/D133283