rjmccall added a comment.
Thanks, I think this approach is really improving the existing code.
================
Comment at: clang/include/clang/AST/GlobalDecl.h:40
+ Stub = 1,
+};
+
----------------
The attribute here is `CUDAGlobalAttr`; should this be named in terms of CUDA,
or is the CUDA model sufficiently different from HIP that the same
implementation concept doesn't apply?
================
Comment at: clang/include/clang/AST/GlobalDecl.h:52
+ assert((!D || !isa<CXXConstructorDecl>(D)) && "Use other ctor with ctor
decls!");
+ assert((!D || !isa<CXXDestructorDecl>(D)) && "Use other ctor with dtor
decls!");
----------------
This function exists primarily to be used as a common initializer for all the
constructors that don't require any of the extra fields. (This file predates
LLVM's adoption of C++14, which allows constructors to delegate to other
constructors.) That's why it asserts that it's not used with constructors or
destructors. So, two questions:
- Is there a reason this function now needs to tolerate null declarations?
- There's some subtle implicit behavior here, in that references to kernels
default to `HIPKernelKind::Kernel`. Is that reasonable, or should there be a
third assertion that this function isn't used with kernel declarations?
================
Comment at: clang/include/clang/AST/GlobalDecl.h:131
+ operator bool() const { return getAsOpaquePtr(); }
+
----------------
`explicit`, please.
================
Comment at: clang/include/clang/AST/GlobalDecl.h:162
+ getDecl()) &&
+ !cast<FunctionDecl>(getDecl())->hasAttr<CUDAGlobalAttr>() &&
!isa<CXXConstructorDecl>(getDecl()) &&
----------------
The indentation here seems odd.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:401
: Context(C), Out(Out_), Structor(getStructor(D)), StructorType(Type),
SeqID(0), AbiTagsRoot(AbiTags) { }
----------------
Does passing down a `GlobalDecl` everywhere allow us to remove these
constructors, i.e. to eliminate the `Structor` and `StructorType` fields?
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:645
+void CXXNameMangler::mangle(GlobalDecl GD) {
+ const NamedDecl *D = dyn_cast<NamedDecl>(GD.getDecl());
// <mangled-name> ::= _Z <encoding>
----------------
This can just be `cast`, except actually I don't think you need a cast here at
all given the code below.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:818
- return nullptr;
+ return GlobalDecl::getFromOpaquePtr(nullptr);
}
----------------
There's a default constructor.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1597
+ else if (auto *DD = dyn_cast<CXXDestructorDecl>(DC))
+ DCGD = GlobalDecl(DD, Dtor_Complete);
+ else
----------------
The relevant wording from the Itanium spec here is:
> For entities in constructors and destructors, the mangling of the complete
> object constructor or destructor is used as the base function name, i.e. the
> C1 or D1 version.
But you might consider pulling this out as a helper function, something like:
```
static GlobalDecl getParentOfLocalEntity(const DeclContext *DC);
```
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1599
+ else
+ DCGD = GlobalDecl(dyn_cast<FunctionDecl>(DC));
+ mangleFunctionEncoding(DCGD);
----------------
This can still be `cast`.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1899
= Template.getAsOverloadedTemplate()) {
- mangleUnqualifiedName(nullptr, (*Overloaded->begin())->getDeclName(),
+ mangleUnqualifiedName(static_cast<NamedDecl *>(nullptr),
(*Overloaded->begin())->getDeclName(),
UnknownArity, nullptr);
----------------
You can just pass `GlobalDecl()` here.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4986
assert(!isa<CXXConstructorDecl>(D) && !isa<CXXDestructorDecl>(D) &&
"Invalid mangleName() call on 'structor decl!");
----------------
The second assertion can just be removed now, since the GD should be carrying
the right information.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1028
else
- MC.mangleName(ND, Out);
+ MC.mangleName(GD, Out);
} else {
----------------
Let's see if we can make this breakdown no longer necessary, since
`MangleContext::mangleName` should be capable of doing the right thing starting
straight from a GD. In fact, maybe we can remove most of the specialized
mangling methods (like `mangleCXXCtor` and `mangleCXXDtor`) from
`MangleContext` completely?
Unrelatedly: there's an `Out` declared in the outermost scope, but a bunch of
these scopes declare their own `Out`; could you just fix that while you're
editing this function?
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1041
+ llvm::raw_svector_ostream Out(Buffer);
+ Out << "__device_stub__" << II->getName();
} else {
----------------
Is this the best way of handling this, or should `shouldMangleDeclName` return
true for kernels (or at least stubs?) even in C? Honest question.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3351
+ } else if (cast<FunctionDecl>(GD.getDecl())->hasAttr<CUDAGlobalAttr>())
+ GD = GD.getWithHIPKernelKind(LangOpts.CUDAIsDevice ? HIPKernelKind::Kernel
: HIPKernelKind::Stub);
+
----------------
Should this be handled in the caller, or would that make things unreasonably
difficult?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68578/new/
https://reviews.llvm.org/D68578
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
