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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
