[PATCH] D41698: [DebugInfo] Enable debug information for C99 VLA types

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen marked 3 inline comments as done.
sdesmalen added inline comments.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2358
+    if (auto *SizeNode = getVLASizeExpressionForType(EltTy))
+      Subscripts.push_back(DBuilder.getOrCreateSubrange(0, SizeNode));
+    else
----------------
aprantl wrote:
> perhaps write sizeNode/Count to a variable, so you don't have to duplicate the expression?
Unfortunately this is necessary because SizeNode and Count are different types, and therefore map to a different function signature of getOrCreateSubrange()


================
Comment at: lib/CodeGen/CGDebugInfo.h:86
+  /// represented by instantiated Metadata nodes.
+  llvm::SmallDenseMap<QualType, llvm::Metadata *> SizeExprCache;
+
----------------
aprantl wrote:
> I'm expecting VLA's to not be very common, should we use a regular DenseMap instead?
Is that not the argument to use a SmallDenseMap instead? The documentation for DenseMap says:
"Also, because DenseMap allocates space for a large number of key/value pairs (it starts with 64 by default), it will waste a lot of space if your keys or values are large."

There is no documentation for SmallDenseMap, but the name suggests it would be optimized for maps with only a few values?


================
Comment at: lib/CodeGen/CGDebugInfo.h:317
+  llvm::Metadata *getVLASizeExpressionForType(QualType Ty) {
+    if (SizeExprCache.count(Ty))
+      return SizeExprCache[Ty];
----------------
aprantl wrote:
> This is performing the lookup twice. If you use .find() instead it will be more efficient. We also don't use accessor functions like this for any of the other caches. If you think that they help, perhaps make this more generic and useful for all caches?
Thanks, I've now removed the accessor function and replaced its uses with the SizeExprCache.find() method as you suggested.


================
Comment at: lib/CodeGen/CGDecl.cpp:1125
+    // If we have debug info enabled, describe the VLA dimensions properly.
+    if (EmitDebugInfo) {
+      QualType Type1D = Ty;
----------------
aprantl wrote:
> Please move this code into a member function of CGDebugInfo.
I moved this to a separate method in CodeGenFunction instead, since it also has to create the actual store (next to creating the DebugInfo), which I don't think should be in CGDebugInfo. Let me know what you think.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1963
+  assert(VlaSize->getType() == SizeTy);
+  return std::pair<llvm::Value *, QualType>(VlaSize, Vla->getElementType());
+}
----------------
aprantl wrote:
> `return {VlaSize, Vla->getElementType()};`
Thanks for the suggestion, that looks much better!


https://reviews.llvm.org/D41698