[Openmp-commits] [PATCH] D101030: [OpenMP] Overhaul `declare target` handling

Tue May 4 07:57:12 PDT 2021

jdoerfert added inline comments.

================
Comment at: clang/lib/CodeGen/CGDecl.cpp:2618
+void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) {
+  for (Expr *E : const_cast<OMPAllocateDecl *>(D)->varlists()) {
+    auto *DE = cast<DeclRefExpr>(E);
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > Why need to remove constantness here?
> > > > > > The problem is that the declaration, which might have been already generated in IR or not, had no address space attached before. Now we want to do that after the fact, potentially *way later*. To reuse the general codegen mechanism I modify the VarDecl (temporarily) by attaching the new address space and calling codegen again. If you have a better idea, I'm happy to change this.
> > > > > Why not just create it manually rather than rely on `GetOrCreateLLVMGlobal`?
> > > > I'm not sure I follow. Creating a global is complicated, need to make sure all the linkage, visibility, address space stuff is set up, mangling, etc. Why would we duplicate all that code and risk messing things up now or in the future?
> > > And what about just setting the address-space on the LLVM type directly?
> > It's part of the type, we can't change it after the fact (at least that is not an existing API). 
> What about `Value::mutateType`?
I'll give it a shot, though the comment makes me nervous:

```
    /// Mutate the type of this Value to be of the specified type.                                                                                                                                                                                             
    ///
    /// Note that this is an extremely dangerous operation which can create
    /// completely invalid IR very easily.  It is strongly recommended that you
    /// recreate IR objects with the right types instead of mutating them in
    /// place.
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101030