[PATCH] D69322: [hip][cuda] Enable extended lambda support on Windows.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:395-400
     /// The number used to indicate this lambda expression for name
     /// mangling in the Itanium C++ ABI.
     unsigned ManglingNumber : 31;
 
+    /// The device side mangling number.
+    unsigned DeviceManglingNumber = 0;
----------------
It seems a shame to grow LambdaDefinitionData by a pointer for all users of C++ that do not use CUDA. Optimizing bitfields may be worth the time, but I'll leave it to @rjmccall or @rsmith to give guidance on whether that's worth it.

An alternative would be to store the device numbers in the mangling context and look them up when needed, since they are so rarely needed.


================
Comment at: clang/include/clang/AST/MangleNumberingContext.h:57-58
+
+  /// Has device mangle numbering context.
+  virtual bool hasDeviceMangleNumberingContext() const { return false; }
+
----------------
hliao wrote:
> rnk wrote:
> > It would be nicer if there were a single entry point that does the right thing for all mangling contexts.
> could you elaborate in more details?
I'll add comments at the call site.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:479-481
+    if (MCtx->hasDeviceMangleNumberingContext()) {
+      Class->setDeviceLambdaManglingNumber(
+          MCtx->getDeviceManglingNumber(Method));
----------------
Rather than having a virtual method that returns bool, call a virtual method that does the entire thing you are trying to do. For example, MSHIP* could be responsible for setting the mangling number on the class. Although, maybe it should be storing the number in some side table now that I think about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69322