[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+                                              QualType Type) {
+  static llvm::DenseMap<QualType, StringRef> Types;
   if (Types.empty()) {
----------------
yihanaa wrote:
> erichkeane wrote:
> > Instead of this initialization, can we make this a constexpr constructor/collection of some sort and instantiate it inline?  
> > Instead of this initialization, can we make this a constexpr constructor/collection of some sort and instantiate it inline?  
> 
> I don't have a good idea because it relies on Context to get some types like const char *
We could at minimum do a in inline-initializer instead of the branch below.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+  if (Types.find(Type) == Types.end())
+    return Types[Context.VoidPtrTy];
+  return Types[Type];
----------------
yihanaa wrote:
> erichkeane wrote:
> > When can we hit this case?  Does this mean that any types we dont understand we are just treating as void ptrs?  
> The previous version treated it as a void *ptr for unsupported types, such as arrays (before this patch) and __int128. This is the case i saw in an issue. I also think that for unsupported types, we should generate a clearer message saying "This type is temporarily not supported", do you have any good ideas?
A diagnostic about ''TYPE' not yet supported by __bultin...' probably isn't a bad idea if we have the ability.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2257
+
+    if (CanonicalType->isConstantArrayType()) {
+      GString = CGF.Builder.CreateGlobalStringPtr(
----------------
yihanaa wrote:
> erichkeane wrote:
> > Do we want to do anything for the OTHER array types?  
> I understand struct fields must have a constant size, and I see" 'variable length array in structure' extension will never be supported" from clang's diagnostics
Ah, right, good point.  I think this + the diagnostic discussed above would make that alright.

There is ALSO the 'matrix' type that we could emit, but that might be worth a separate commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122822