[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct
From:       Wang Yihan via Phabricator via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2022-03-31 19:02:42
Message-ID: ex-sXlYKRXu_PfkAlI-l9A () geopod-ismtpd-4-0
[Download RAW message or body]

yihanaa added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122822

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]