[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:
> > yihanaa wrote:
> > > erichkeane wrote:
> > > > yihanaa wrote:
> > > > > yihanaa wrote:
> > > > > > erichkeane wrote:
> > > > > > > yihanaa wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > 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.
> > > > > > > > I still haven't come up with a good idea to do it😔
> > > > > > > Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> > > > > > > 
> > > > > > > Since this is a finite, known at compile time list of low 'N', I'd suggest just making it `static std::array<pair<QualType, StringRef>> Types = {{Context.CharTy, "%c"},...`.
> > > > > > > 
> > > > > > > Then just doing a llvm::find_if.  The list is small enough I suspect the DenseMap overhead is more than the use of `llvm::find_if`.
> > > > > > Haha,DenseMap also has no constexpr ctor. I think the time complexity of these two is the same. 
> > > > > The Context.CharTy... etc. are not static, So we might need a static value instead of it, and compare it with QualType
> > > > They don't have to be static?  This should do 'magic static' initialization.
> > > I can't agree more. it might be a type identifier, such as getAsString()'s return value
> > Actually, I don't think the collection can be static!  When we re-use the same process for multiple TUs, we get a new ASTContext.  SO this would result in us having types that don't necessarily 'match'.  I think this collection/checks need to either be stored in ASTContext, or in an 'if-else' tree here.
> I also don't think it should be maintained in this place. Shouldn't this be better maintained together with the printf parameter checking part, what do you think?
I'm unfamiliar with what you mean.

HOWEVER, the comments by @rsmith and @rjmccall I think are blocking on this feature/functionality anyway.  So we might still wish to refactor this builtin significantly based on what they come up with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122822