[PATCH] D122734: [CUDA][HIP] Fix mangling number for local struct

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:682
+    /// Current name mangling is for device name in host compilation.
+    bool MangleDeviceNameInHostCompilation = false;
+  } CUDANameMangleCtx;
----------------
rnk wrote:
> yaxunl wrote:
> > rnk wrote:
> > > It doesn't feel right to store mutable state here in ASTContext. You are effectively using this as a global variable to thread a boolean through to `ASTContext::getManglingNumber`, which is called from the mangler. The boolean indicates of host or device mangling numbers are desired for this particular mangling. Is that more or less correct?
> > > 
> > > This makes me think that it was perhaps a mistake to store mangling numbers in the AST Context in the first place. If we stored them in the mangling context instead, I think we'd get the right behavior out of the box. Is that correct? It it feasible to make that change?
> > > It doesn't feel right to store mutable state here in ASTContext. You are effectively using this as a global variable to thread a boolean through to `ASTContext::getManglingNumber`, which is called from the mangler. The boolean indicates of host or device mangling numbers are desired for this particular mangling. Is that more or less correct?
> > > 
> > > This makes me think that it was perhaps a mistake to store mangling numbers in the AST Context in the first place. If we stored them in the mangling context instead, I think we'd get the right behavior out of the box. Is that correct? It it feasible to make that change?
> > 
> > It is possible but I feel it is too invasive. Mangling numbers need to be calculated during construction of AST since it is context dependent. Mangle contexts are created in ABI of codegen. To store mangling numbers in mangling contexts, we need to move mangling contexts from ABI to ASTContext. We also need to add device mangling context in ASTContext. Then for each use of get/set mangling number API, we need to add get/set device mangling number API. That would need lots of changes.
> > 
> > I would see the state added to ASTContext by my approach from the perspective of single source language. The program contains both device code and host code. Ideally the compiler should be able to compile the device code with device target and ABI and compile the host code with host target ABI and switch the target freely during codegen. When we try to get the device side mangled name in host compilation, what we trying to do is switch to device target momoentorily during the mangling. This needs a state in ASTContext indicating that we are doing mangling for device instead of host, therefore getManglingNumber should return device mangling number instead of host mangling number.
> After auditing call sites for setManglingNumber, I see that many of them relate to template instantiation and AST serialization. With that in mind, I think it makes sense to leave the ManglingNumbers DenseMap as you have it, it's not worth updating all of those serialization and cloning codepaths.
> 
> However, I really would prefer to remove this boolean on the ASTContext, which effectively stands in as an extra parameter to all getManglingNumber calls. The manglers should be able to calculate this boolean when they are created (they should know if they are the Itanium device mangler or the MS host mangler) and pass the correct value to every call to ASTContext::getManglingNumber. Does that make sense?
> 
> However, I really would prefer to remove this boolean on the ASTContext, which effectively stands in as an extra parameter to all getManglingNumber calls. The manglers should be able to calculate this boolean when they are created (they should know if they are the Itanium device mangler or the MS host mangler) and pass the correct value to every call to ASTContext::getManglingNumber. Does that make sense?

Yes. Opened https://reviews.llvm.org/D124842 for this.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122734