[PATCH] D68578: [HIP] Fix device stub name
Yaxun Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 5 10:57:52 PST 2020
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
More information about the cfe-commits
mailing list