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

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thanks for your suggestion @erichkeane !



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+                                              QualType Type) {
+  static llvm::DenseMap<QualType, StringRef> Types;
   if (Types.empty()) {
----------------
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 *


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+  if (Types.find(Type) == Types.end())
+    return Types[Context.VoidPtrTy];
+  return Types[Type];
----------------
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?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2119
+  // But if the array length is constant, we can suppress that.
+  if (llvm::ConstantInt *CL = dyn_cast<llvm::ConstantInt>(Length)) {
+    // ...and if it's constant zero, we can just skip the entire thing.
----------------
erichkeane wrote:
> This branch doesn't seem right?  We are in a `ConstantArrayType`, which means we DEFINITELY know the length.  Additionally, `Length` is constructed directly from a `llvm::ConstantInt::get`, so we already know this answer.
I agree with you


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2138
+
+  if (CheckZeroLength) {
+    llvm::Value *isEmpty =
----------------
erichkeane wrote:
> We already know if this is a 'zero' length array, we should probably just not emit IR at all in that case.
Good idea!


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+  if (inArray) {
+    GString = CGF.Builder.CreateGlobalStringPtr("{\n");
+  } else {
----------------
erichkeane wrote:
> instead of `inArray` you probably should have something a little more descriptive here, it seems what you really need is something like, `includeTypeName`, preferably as an enum to make it more clear at the caller side.
I think that's a good idea for clarity.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2257
+
+    if (CanonicalType->isConstantArrayType()) {
+      GString = CGF.Builder.CreateGlobalStringPtr(
----------------
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


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2827
     LValue RecordLV = MakeAddrLValue(RecordPtr, Arg0Type, Arg0Align);
-    Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align,
+    Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align, false,
                             {LLVMFuncType, Func}, 0);
----------------
erichkeane wrote:
> Typically we do a comment to explain what bool parms mean.  Alternatively, if instead you could create an enum of some sort that would better describe the 'false', it wouldlikely be more helpful.  (same on 2252).
I agree!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122822