rjmccall added inline comments.
================
Comment at: clang/lib/Sema/Sema.cpp:1514
+ void visitUsedDecl(SourceLocation Loc, Decl *D) {
+ if (auto *TD = dyn_cast<TranslationUnitDecl>(D)) {
+ for (auto *DD : TD->decls()) {
----------------
erichkeane wrote:
> rjmccall wrote:
> > erichkeane wrote:
> > > Note that when recommitting this (if you choose to), this needs to also
> > > handle NamespaceDecl. We're a downstream and discovered that this
> > > doesn't properly handle functions or records handled in a namespace.
> > >
> > > It can be implemented identically to TranslationUnitDecl.
> > Wait, what? We shouldn't be doing this for TranslationUnitDecl either. I
> > don't even know how we're "using" a TranslationUnitDecl, but neither this
> > case not the case for `NamespaceDecl` should be recursively using every
> > declaration declared inside it. If there's a declaration in a namespace
> > that's being used, it should be getting visited as part of the actual use
> > of it.
> >
> > The logic for `RecordDecl` has the same problem.
> Despite the name, this seems to be more of a home-written ast walking class.
> The entry point is the 'translation unit' which seems to walk through
> everything in an attempt to find all the functions (including those that are
> 'marked' as used by an attribute).
>
> You'll see the FunctionDecl section makes this assumption as well (not
> necessarily that we got to a function via a call). IMO, this approach is
> strange, and we should register entry points in some manner (functions marked
> as emitted to the device in some fashion), then just follow its call-graph
> (via the clang::CallGraph?) to emit all of these functions.
>
> It seemed really odd to see this approach here, but it seemed well reviewed
> by the time I noticed it (via a downstream bug) so I figured I'd lost my
> chance to disagree with the approach.
>
>
Sure, but `visitUsedDecl` isn't the right place to be entering the walk.
`visitUsedDecl` is supposed to be the *callback* from the walk. If they need
to walk all the global declarations to find kernels instead of tracking the
kernels as they're encountered (which would be a *much* better approach), it
should be done as a separate function.
I just missed this in the review.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70172/new/
https://reviews.llvm.org/D70172
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
