yaxunl marked 38 inline comments as done.
yaxunl added inline comments.
================
Comment at: clang/include/clang/AST/GlobalDecl.h:40
+ Stub = 1,
+};
+
----------------
tra wrote:
> rjmccall wrote:
> > tra wrote:
> > > rjmccall wrote:
> > > > tra wrote:
> > > > > rjmccall wrote:
> > > > > > 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?
> > > > > I believe the attribute serves the same purpose in both CUDA and HIP
> > > > > and could be renamed appropriately in a separate patch.
> > > > >
> > > > > While the changes in this patch are not required for CUDA, CUDA would
> > > > > benefit from them. We could use a generic GPU prefix and migrate CUDA
> > > > > to the same model later. A TODO comment about that would be nice.
> > > > I'd just like consistency. If they're serving the same purpose, then
> > > > as someone with no dog in this fight, I would give precedence to CUDA
> > > > over HIP in names since it's both the older language and was
> > > > implemented first in Clang (even if only partially, IIUC). I don't
> > > > think a generic name works unless we can meaningfully generalize it to
> > > > all languages with a similar feature, e.g. OpenCL and so on.
> > > Naming, the hardest problem in computer science. :-)
> > > I personally would prefer generalization-with-exclusions over specific
> > > name which is inconsistently commingles things that's really specific to
> > > that name and things that are more widely used. Alas, right now CUDA is
> > > the example of the latter case -- some parts are CUDA-specific and a lot
> > > are shared with HIP.
> > >
> > > For the new features we've been sort of sticking with using CUDA/HIP for
> > > specific parts and GPU for shared functionality, but as things are a lot
> > > of shared bits are still 'CUDA' and it's hard to tell them apart. As you
> > > point it out, renaming the incumbent names would be a pain, so here we
> > > are.
> > >
> > > I think using `GPUKernelKind` with a comment that it reflects HIP & CUDA
> > > kernels would be somewhat better choice than `CUDAKernelKind` which would
> > > be somewhat confusing at this point given that CUDA actually does not use
> > > it yet. I'm also fine with keeping it `HIPKernelKind` and postpone the
> > > naming decision until CUDA-related parts are actually implemented.
> > Maybe `KernelReferenceKind`? It's probably a common concept across all
> > heterogenous-computing models.
> SGTM.
changed.
================
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!");
----------------
rjmccall wrote:
> 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?
By using default constructor of GlobalDecl, null declaration is no longer
necessary. I have reverted change to assert.
I think it is a good idea to force GlobalDecl of kernels to be instantiated
with the specific ctor. I have added assert to Init to prevent kernels to be
instantiated through it.
================
Comment at: clang/include/clang/AST/GlobalDecl.h:131
+ operator bool() const { return getAsOpaquePtr(); }
+
----------------
rjmccall wrote:
> `explicit`, please.
fixed
================
Comment at: clang/include/clang/AST/GlobalDecl.h:162
+ getDecl()) &&
+ !cast<FunctionDecl>(getDecl())->hasAttr<CUDAGlobalAttr>() &&
!isa<CXXConstructorDecl>(getDecl()) &&
----------------
rjmccall wrote:
> The indentation here seems odd.
fixed
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:401
: Context(C), Out(Out_), Structor(getStructor(D)), StructorType(Type),
SeqID(0), AbiTagsRoot(AbiTags) { }
----------------
rjmccall wrote:
> Does passing down a `GlobalDecl` everywhere allow us to remove these
> constructors, i.e. to eliminate the `Structor` and `StructorType` fields?
Removing eliminate the Structor and StructorType will incur significantly more
changes. Can it be done later? Thanks.
================
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>
----------------
rjmccall wrote:
> This can just be `cast`, except actually I don't think you need a cast here
> at all given the code below.
removed cast
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:818
- return nullptr;
+ return GlobalDecl::getFromOpaquePtr(nullptr);
}
----------------
rjmccall wrote:
> There's a default constructor.
fixed
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1597
+ else if (auto *DD = dyn_cast<CXXDestructorDecl>(DC))
+ DCGD = GlobalDecl(DD, Dtor_Complete);
+ else
----------------
rjmccall wrote:
> 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);
> ```
extracted to getParentOfLocalEntity
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:1599
+ else
+ DCGD = GlobalDecl(dyn_cast<FunctionDecl>(DC));
+ mangleFunctionEncoding(DCGD);
----------------
rjmccall wrote:
> This can still be `cast`.
fixed
================
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);
----------------
rjmccall wrote:
> You can just pass `GlobalDecl()` here.
fixed
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4986
assert(!isa<CXXConstructorDecl>(D) && !isa<CXXDestructorDecl>(D) &&
"Invalid mangleName() call on 'structor decl!");
----------------
rjmccall wrote:
> The second assertion can just be removed now, since the GD should be carrying
> the right information.
removed
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1028
else
- MC.mangleName(ND, Out);
+ MC.mangleName(GD, Out);
} else {
----------------
rjmccall wrote:
> tra wrote:
> > rjmccall wrote:
> > > 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?
> > Perhaps it would make sense to split this patch into two -- one that
> > changes mangler input to GlobalDecl and the other one dealing with HIP
> > stubs.
> Yes, that's a good idea.
Fixed the redundant Out var.
However, removing mangleCXXCtor/Dtor will incur significantly more changes. Can
it be done later? Thanks.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1028
else
- MC.mangleName(ND, Out);
+ MC.mangleName(GD, Out);
} else {
----------------
yaxunl wrote:
> rjmccall wrote:
> > tra wrote:
> > > rjmccall wrote:
> > > > 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?
> > > Perhaps it would make sense to split this patch into two -- one that
> > > changes mangler input to GlobalDecl and the other one dealing with HIP
> > > stubs.
> > Yes, that's a good idea.
> Fixed the redundant Out var.
>
> However, removing mangleCXXCtor/Dtor will incur significantly more changes.
> Can it be done later? Thanks.
separated to https://reviews.llvm.org/D75700
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1041
+ llvm::raw_svector_ostream Out(Buffer);
+ Out << "__device_stub__" << II->getName();
} else {
----------------
rjmccall wrote:
> Is this the best way of handling this, or should `shouldMangleDeclName`
> return true for kernels (or at least stubs?) even in C? Honest question.
This is for extern "C" kernels, which are either not mangled or with simple
prefix. I tried returning true for them in shouldMangleDeclName, and they got
mangled as Itanium mangling, which seems not right.
================
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);
+
----------------
rjmccall wrote:
> Should this be handled in the caller, or would that make things unreasonably
> difficult?
fixed
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
