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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
