Page MenuHomePhabricator

[hip][cuda] Enable extended lambda support on Windows.
Needs ReviewPublic

Authored by hliao on Tue, Oct 22, 2:04 PM.

Details

Summary
  • On Windows, extended lambda has extra issues due to the numbering schemes are different between the host compilation (Microsoft C++ ABI) and the device compilation (Itanium C++ ABI. Additional device side lambda number is required per lambda for the host compilation to correctly mangle the device-side lambda name.
  • A hybrid numbering context MSHIPNumberingContext is introduced to number a lambda for both host- and device-compilations.

Event Timeline

hliao created this revision.Tue, Oct 22, 2:04 PM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
hliao added a comment.Wed, Oct 23, 1:23 PM

PING for review

Please don't ping for review after a single day.

hliao added a comment.Tue, Oct 29, 5:40 AM

PING for review

tra added a reviewer: rnk.Tue, Oct 29, 9:10 AM
tra added a subscriber: rnk.

@rnk Reid, would you be the right person to look at the change on the Windows' side?

rnk added inline comments.Tue, Oct 29, 4:31 PM
clang/include/clang/AST/MangleNumberingContext.h
57–58

It would be nicer if there were a single entry point that does the right thing for all mangling contexts.

clang/lib/AST/MicrosoftCXXABI.cpp
67–68

I know it's inheritance instead of composition, but maybe the simple thing to do here would be to inherit from the MicrosoftMangleNumberingContext, and then implement an override for getDeviceManglingNumber that delegates to the Itanium one.

89

This is actually the device's mangling context, and it's an Itanium mangling context. I think it needs comments and a more descriptive variable name.

martong resigned from this revision.Wed, Oct 30, 2:23 AM
hliao updated this revision to Diff 227091.Wed, Oct 30, 7:14 AM

revise MSHIPNumberingContext

hliao marked 3 inline comments as done.Wed, Oct 30, 7:16 AM
hliao added inline comments.
clang/include/clang/AST/MangleNumberingContext.h
57–58

could you elaborate in more details?

rnk added inline comments.Wed, Oct 30, 11:46 AM
clang/include/clang/AST/DeclCXX.h
395–402

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.

clang/include/clang/AST/MangleNumberingContext.h
57–58

I'll add comments at the call site.

clang/lib/Sema/SemaLambda.cpp
479–481

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.

hliao updated this revision to Diff 227214.Wed, Oct 30, 7:53 PM
hliao marked an inline comment as done.

simplify again following suggestion.

hliao added inline comments.Wed, Oct 30, 7:54 PM
clang/include/clang/AST/DeclCXX.h
395–402

I like the alternative way by storing all numbering into the mangle/numbering context instead of AST itself. But, it needs additional numbering post-processing after AST importing. Sound to me a major refactoring work likely to be addressed later.

hliao added a comment.Fri, Nov 1, 9:59 AM

kindly PING for review

hliao added a comment.Mon, Nov 4, 2:00 PM

Looks like holidays are approaching, :). Anyway, it's really appreciated that you could review this patch to enable CUDA/HIP applications on Windows.

Thanks.

hliao added a comment.Wed, Nov 6, 5:56 AM

PING for review

hliao added a comment.Fri, Nov 8, 9:01 AM

PING for review