[LIBCLC] Generate FunctionDecl to remangle substituted entries #7220

jchlanda · 2022-10-28T13:59:50Z

Retire the by hand mangling of substituted function in favor of constructing corresponding FunctionDecls and letting clang mangle it for us. This ensures that we never diverge from clang's mangling idiosyncrasies and bugfixes would likely be in the AST creation, not mangling itself.

The tool has been rewritten as a ClangTool implementing FrontendAction.

Additionally, we provide the option to perform a test run, in which no substitutions are made, while checking that remangled name does not diverge from the original mangled name. The tests are added to check-libclc target.

The diffs are a bit ugly, it might be easier to inspect the file mode, basically the change lives entirely in Remangler class, and if you follow Remangler::remangle it will lead you through what it does.

Fixes: #6505

jchlanda · 2022-10-28T14:00:17Z

@steffenlarsen you might find it interesting :)

romanovvlad · 2022-11-14T09:43:40Z

@bader Could you please help with review?

Naghasan · 2022-11-14T10:22:27Z

Can I suggest someone from @intel/dpcpp-cfe-reviewers help for this ? This PR is mainly playing with the clang tooling so they would be the most relevant.

bader · 2022-11-15T09:16:49Z

@jchlanda, could you resolve merge conflicts, please?

jchlanda · 2022-11-15T10:06:53Z

@jchlanda, could you resolve merge conflicts, please?

Done.

jchlanda · 2022-11-15T14:12:08Z

Fixes: #6505

bader · 2022-11-16T10:45:36Z

Can I suggest someone from @intel/dpcpp-cfe-reviewers help for this ? This PR is mainly playing with the clang tooling so they would be the most relevant.

@intel/dpcpp-cfe-reviewers, ping.

bader · 2022-11-16T10:47:17Z

Fixes: #6505

If you add this line to the PR description, GitHub will automatically link the PR with the referenced issue.

smanna12 · 2022-11-16T15:17:04Z

Can I suggest someone from @intel/dpcpp-cfe-reviewers help for this ? This PR is mainly playing with the clang tooling so they would be the most relevant.

@intel/dpcpp-cfe-reviewers, ping.

I am not very familiar with the ClangTool. @premanandrao, @AaronBallman, could you please take a look at?

AaronBallman

I don't have much expertise in this area, so a lot of my comments are more superficial.

Does existing test coverage suffice to test these changes?

libclc/utils/libclc-remangler/LibclcRemangler.cpp

AaronBallman · 2022-11-16T16:39:34Z

libclc/utils/libclc-remangler/LibclcRemangler.cpp

+    // Fix up the template types in the original FD's arg tys and return ty.
+    auto AreQTsEqual = [&](clang::QualType &LHS, clang::QualType &RHS) -> bool {
+      return (LHS.getBaseTypeIdentifier() && RHS.getBaseTypeIdentifier() &&
+              LHS.getBaseTypeIdentifier()->isStr(
+                  RHS.getBaseTypeIdentifier()->getName())) ||
+             LHS == RHS;
+    };
+    unsigned NumReplaced = 0;
+    unsigned Idx = 0;
+    for (auto &TemplateArgQT : TemplateArgTys) {
+      if (AreQTsEqual(TemplateArgQT, RetTy)) {
+        RetTy = TemplateTypeParamTys[Idx];
+        goto Found;
+      }
+      for (unsigned i = 0; i < ArgTys.size(); ++i) {
+        if (AreQTsEqual(ArgTys[i], TemplateArgQT)) {
+          ArgTys[i] = TemplateTypeParamTys[Idx];
+          goto Found;
+        }
+      }


Are you basically trying to instantiate the function here?

Yeap, making sure that the template is fully specialized.

Ooofda, I think you might need to use the usual template instantiation machinery to do this (you may have multiple levels of instantiation to perform for a type like foo<T<U>> (think: template template parameters). CC @erichkeane

This is a bit of a tricky issue.
In principle you are right; the following, fictional, overload of image read would not be handled correctly:

template <typename T> class woof{}; template <> int4 __spirv_ImageRead<int4 , ocl_image3d_ro, woof<int4>>(ocl_image3d_ro, woof<int4>) {}

However, this patch:

does not deviate from the current status, the same failure could be observed in the mainline,

we only need to be able to handle functions defined in libclc, all of which have to follow spir-v interface,

I am concerned that if we wanted to make it complete, we would end up re-implementing huge chunks of clang for something that is a closed scope task,

finally, remangler will hard fail should the interface changed.

With thte patch, the above sample would result in:

Unhandled type : woof<int vector[4]> UNREACHABLE executed at /home/jakub/Projects/oneAPI/jchlanda/llvm/src/llvm/frontendaction/LibclcRemangler.cpp:479!

Ooof, there are quite a lot of situations that'll get out of hand here, its not handling constraint overloading, template-template params, etc, but I don't have a good hold on it. BUT if the libclc owner is equally unconcerned as you about these, I've got no problems obviously.

@steffenlarsen would you be able to weigh in on this?

Use of templates in the current builtins are luckily limited and hopefully they stay this way. It would be nice to see handling of these improve in the future (hopefully in a way where it can again reuse some existing logic) but at least this doesn't seem to worsen the handling of these outliers compared to mainline.

Note that the builtins libclc attempts to mimic need to be handled correctly by the SPIR-V Translator too, so to my knowledge the subset of possible instantiations we support of the templated builtins are actually fairly limited too.

AaronBallman · 2022-11-16T16:44:09Z

libclc/utils/libclc-remangler/LibclcRemangler.cpp

+      assert(false && "unhandled type name: decltype(auto)");
+    else if (Name == "std::nullptr_t")
+      Res = ContextAST->NullPtrTy;
+    else {


So if you don't know the type you assume it's an enumeration type? That seems dangerous. For example, this is missing a type for _BitInt, so that would turn into an enum here. I think the else case should be an assertion to help spot missing types.

Although this matches what the previous code did.

I've added an assert for _BitInt, the problem here is that we match on strings printed from itanium_demangle::Node and having a generic else close that matches an invalid state is not possible (or hard to get?). FWIW handling a valid qualified type as an enum (falling to catch all else as it is now) would result in a mis-mangled name and would be spotted by the tests.

Doesn't that presume we're testing every possible builtin type? (That sounds like it would get out of sync pretty quickly as we do add new builtin types, though somewhat uncommonly.)

This is similar to #7220 (comment), we only need to be able to handle what spir-v can throw at the remangler.

premanandrao · 2022-11-16T20:07:07Z

libclc/utils/libclc-remangler/LibclcRemangler.cpp

+    BlockMeta *NewMeta = reinterpret_cast<BlockMeta *>(std::malloc(NBytes));
+    if (NewMeta == nullptr)
+      std::terminate();
+    BlockList->Next = new (NewMeta) BlockMeta{BlockList->Next, 0};


@AaronBallman, does this statement have sequencing issues or is it well sequenced?

Okay, so I am glad I was not the only person who looked at that code and went "is that right?"! :-D

There's no sequencing problem there because of: https://eel.is/c++draft/expr.ass#1

libclc/utils/libclc-remangler/LibclcRemangler.cpp

premanandrao · 2022-11-16T20:23:34Z

libclc/utils/libclc-remangler/LibclcRemangler.cpp

+      assert(false && "unhandled type name: decltype(auto)");
+    else if (Name == "std::nullptr_t")
+      Res = ContextAST->NullPtrTy;
+    else {


Although this matches what the previous code did.

premanandrao · 2022-11-16T20:34:11Z

libclc/utils/libclc-remangler/LibclcRemangler.cpp

+
+  bool remangleFunction(Function &Func, llvm::Module *M) {
+    if (!Func.getName().startswith("_Z"))
+      return true;


Should this return false instead?

I don't think so, the bool here is used as a success/failure notion, and I'd say that a state where we don't need to remangle and can early exit from this function matches the success.

premanandrao · 2022-11-16T20:36:06Z

I am not very familiar with the ClangTool. @premanandrao, @AaronBallman, could you please take a look at?

Sorry @smanna12, me neither.

jchlanda · 2022-11-18T06:30:38Z

I don't have much expertise in this area, so a lot of my comments are more superficial.

Does existing test coverage suffice to test these changes?

As it stands now we do not test remangler and discover the failures at runtime with ptxas reporting unresolved function calls (for example: #6505); this PR brings testing capability to the remangler, by providing a -t (test run) option; when used, the remangler will not perform any substitutions and assert that the remangled name matches the input one; this is wrapped against check-sycl target here: https://github.com/intel/llvm/pull/7220/files#diff-023be2cbe5badf0f927377ab972855d2e09923a36976921fd8c1efabb307edeaR57

AaronBallman · 2022-11-18T12:38:47Z

I don't have much expertise in this area, so a lot of my comments are more superficial.
Does existing test coverage suffice to test these changes?

As it stands now we do not test remangler and discover the failures at runtime with ptxas reporting unresolved function calls (for example: #6505); this PR brings testing capability to the remangler, by providing a -t (test run) option; when used, the remangler will not perform any substitutions and assert that the remangled name matches the input one; this is wrapped against check-sycl target here: https://github.com/intel/llvm/pull/7220/files#diff-023be2cbe5badf0f927377ab972855d2e09923a36976921fd8c1efabb307edeaR57

Thank you for the explanation! Are there existing tests for -t that this will start to exercise, or should we add a case to this patch to exercise those changes?

jchlanda · 2022-11-22T10:35:20Z

I don't have much expertise in this area, so a lot of my comments are more superficial.
Does existing test coverage suffice to test these changes?

As it stands now we do not test remangler and discover the failures at runtime with ptxas reporting unresolved function calls (for example: #6505); this PR brings testing capability to the remangler, by providing a -t (test run) option; when used, the remangler will not perform any substitutions and assert that the remangled name matches the input one; this is wrapped against check-sycl target here: https://github.com/intel/llvm/pull/7220/files#diff-023be2cbe5badf0f927377ab972855d2e09923a36976921fd8c1efabb307edeaR57

Thank you for the explanation! Are there existing tests for -t that this will start to exercise, or should we add a case to this patch to exercise those changes?

I'm not sure if I follow exactly, but

if you're asking If the change will be pulled in to the CI and exercised, than yes, we already run check-libclc here and the test run will extend that
if, on the other hand, you mean if -t itself is tested in any way, than no, it is not, however I feel that is OK. The switch itself is very limited in the scope, it does only two things:
** prevents the substitution: https://github.com/jchlanda/llvm/blob/jakub/remangling_action/libclc/utils/libclc-remangler/LibclcRemangler.cpp#L484 this createst the test condition, with no substitution the remangled name must be equal to the original one
** makes sure that we fail on mismatch: https://github.com/jchlanda/llvm/blob/jakub/remangling_action/libclc/utils/libclc-remangler/LibclcRemangler.cpp#L882
I worry that if we wanted to test the Test Run itself we might end up in an infinite loop of adding flags to test flags, as we would somehow need to inject failure into the remangler (probably guarded by a flag).

AaronBallman · 2022-11-22T13:19:48Z

I don't have much expertise in this area, so a lot of my comments are more superficial.
Does existing test coverage suffice to test these changes?

As it stands now we do not test remangler and discover the failures at runtime with ptxas reporting unresolved function calls (for example: #6505); this PR brings testing capability to the remangler, by providing a -t (test run) option; when used, the remangler will not perform any substitutions and assert that the remangled name matches the input one; this is wrapped against check-sycl target here: https://github.com/intel/llvm/pull/7220/files#diff-023be2cbe5badf0f927377ab972855d2e09923a36976921fd8c1efabb307edeaR57

Thank you for the explanation! Are there existing tests for -t that this will start to exercise, or should we add a case to this patch to exercise those changes?

I'm not sure if I follow exactly, but

if you're asking If the change will be pulled in to the CI and exercised, than yes, we already run check-libclc here and the test run will extend that

if, on the other hand, you mean if -t itself is tested in any way, than no, it is not, however I feel that is OK. The switch itself is very limited in the scope, it does only two things:
** prevents the substitution: https://github.com/jchlanda/llvm/blob/jakub/remangling_action/libclc/utils/libclc-remangler/LibclcRemangler.cpp#L484 this createst the test condition, with no substitution the remangled name must be equal to the original one
** makes sure that we fail on mismatch: https://github.com/jchlanda/llvm/blob/jakub/remangling_action/libclc/utils/libclc-remangler/LibclcRemangler.cpp#L882
I worry that if we wanted to test the Test Run itself we might end up in an infinite loop of adding flags to test flags, as we would somehow need to inject failure into the remangler (probably guarded by a flag).

I don't know about libclc, but at least in LLVM and Clang, adding untested code is generally not something we do. But I see now that the CMake changes also include an execution with -t (https://github.com/intel/llvm/pull/7220/files#diff-023be2cbe5badf0f927377ab972855d2e09923a36976921fd8c1efabb307edeaR63) so long as LIBCLC_GENERATE_REMANGLED_VARIANTS is defined. Is that variable on by default or get enabled by a CI run so if the flag was broken (perhaps in the future), at least one test would break as well?

jchlanda · 2022-11-22T14:42:42Z

Yeap, it is being build by default for the CI runs (see the buildbot script here: https://github.com/jchlanda/llvm/blob/jakub/remangling_action/buildbot/configure.py#L127), as well as for both CUDA and HIP.

AaronBallman · 2022-11-22T14:52:37Z

Yeap, it is being build by default for the CI runs (see the buildbot script here: https://github.com/jchlanda/llvm/blob/jakub/remangling_action/buildbot/configure.py#L127), as well as for both CUDA and HIP.

Fantastic, that alleviates my concerns about the testing!

jchlanda · 2022-11-24T08:02:12Z

Is there anything else I could do for this PR?

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Co-authored-by: premanandrao <premanand.m.rao@intel.com>

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

jchlanda · 2022-12-06T10:02:40Z

The failures are not related (assert tests in HIP) are we OK to merge this @intel/llvm-gatekeepers

steffenlarsen · 2022-12-06T12:20:11Z

@premanandrao & @erichkeane & @AaronBallman - Have your concerns been addressed?

premanandrao · 2022-12-06T15:55:47Z

@premanandrao & @erichkeane & @AaronBallman - Have your concerns been addressed?

My concerns were all addressed.

pvchupin · 2022-12-07T00:10:11Z

@jchlanda, please address post-commit issue on Windows: https://github.com/intel/llvm/actions/runs/3634072977/jobs/6131787943

and on Linux in this mode: https://github.com/intel/llvm/actions/runs/3634072977/jobs/6131787547

pvchupin · 2022-12-07T19:00:30Z

@jchlanda, I'm going to revert it in a moment. Let me know if you have any other fix ready.

#7220)" This reverts commit 114aeba.

…s" (#7694) Reverts #7220

@pvchupin

@pvchupin I've fixed the issues you mentioned in #7220 (comment) (the last two commits): * shared libs build: e0f3a52 * windows mangling: b88627f The rest is as per #7220 Co-authored-by: Aaron Ballman <aaron@aaronballman.com> Co-authored-by: premanandrao <premanand.m.rao@intel.com> Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

The diffs are quite hard to follow, but in an essence this patch brings: * a new entry, implementing a generic address space for multiple `__CLC_NVVM_ATOMIC_XYZ_IMPL`, where `XYZ` stands for `CAS`, `INDEC`, `LOAD`, `MAX`, `MIN`, `STORE` and `SUB`, * fixes the name of mangled function that the IMPL uses, * the rest is just formatting to 80 chars. This patch supersedes: #5849 but it requires the fixes to the remangler from: #7220 Fixes: #7658

jchlanda requested a review from a team as a code owner October 28, 2022 13:59

jchlanda requested a review from romanovvlad October 28, 2022 13:59

bader requested a review from a team November 14, 2022 12:43

jchlanda mentioned this pull request Nov 15, 2022

[LIBCLC] Add support for more generic atomic operations #7391

Merged

jchlanda force-pushed the jakub/remangling_action branch from 0ab248c to 3d8d1b2 Compare November 15, 2022 09:58

jchlanda mentioned this pull request Nov 15, 2022

SYCL-CTS test_vector_swizzles doesn't compile for NVPTX target. #6505

Closed

AaronBallman reviewed Nov 16, 2022

View reviewed changes

premanandrao reviewed Nov 16, 2022

View reviewed changes

jchlanda marked this pull request as draft November 18, 2022 08:48

jchlanda marked this pull request as ready for review November 18, 2022 08:49

jchlanda requested review from AaronBallman, erichkeane and steffenlarsen November 24, 2022 08:02

jchlanda and others added 15 commits December 5, 2022 14:59

[LIBCLC] Generate FunctionDecl to remangle substituted entries

f2b0d90

Fix buffer initialization

28fe27a

Update libclc/utils/libclc-remangler/LibclcRemangler.cpp

7be7062

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Update libclc/utils/libclc-remangler/LibclcRemangler.cpp

dbe227c

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Remove dummy namespace

a8edbfa

Whitespace fix

88f9b87

Fix dependencies

0527bbd

Add _BitInt clause

a370e00

Update libclc/utils/libclc-remangler/LibclcRemangler.cpp

addc0d2

Co-authored-by: premanandrao <premanand.m.rao@intel.com>

Update libclc/utils/libclc-remangler/LibclcRemangler.cpp

526dc60

Co-authored-by: premanandrao <premanand.m.rao@intel.com>

Update signature of createKernelDecl

548e0b5

Update AreQTsEqual

ab91615

Use max_align_t for InitialBuffer alignment

93a5c50

Update libclc/utils/libclc-remangler/LibclcRemangler.cpp

06376bf

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

Update libclc/utils/libclc-remangler/LibclcRemangler.cpp

e3bb16e

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>

jchlanda force-pushed the jakub/remangling_action branch from 8e95ddc to e3bb16e Compare December 5, 2022 16:13

Fix signature for NamespaceDecl::Create

5b88745

premanandrao approved these changes Dec 6, 2022

View reviewed changes

pvchupin merged commit 114aeba into intel:sycl Dec 6, 2022

pvchupin pushed a commit that referenced this pull request Dec 7, 2022

Revert "[LIBCLC] Generate FunctionDecl to remangle substituted entries (

eadb31c

#7220)" This reverts commit 114aeba.

pvchupin mentioned this pull request Dec 7, 2022

Revert "[LIBCLC] Generate FunctionDecl to remangle substituted entries" #7694

Merged

pvchupin pushed a commit that referenced this pull request Dec 7, 2022

Revert "[LIBCLC] Generate FunctionDecl to remangle substituted entrie…

62c0643

…s" (#7694) Reverts #7220

jchlanda mentioned this pull request Dec 9, 2022

[SYCL][libclc] Remangler fixes #7723

Merged

[LIBCLC] Generate FunctionDecl to remangle substituted entries #7220

[LIBCLC] Generate FunctionDecl to remangle substituted entries #7220

Uh oh!

Conversation

jchlanda commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jchlanda commented Oct 28, 2022

Uh oh!

romanovvlad commented Nov 14, 2022

Uh oh!

Naghasan commented Nov 14, 2022

Uh oh!

bader commented Nov 15, 2022

Uh oh!

jchlanda commented Nov 15, 2022

Uh oh!

jchlanda commented Nov 15, 2022

Uh oh!

bader commented Nov 16, 2022

Uh oh!

bader commented Nov 16, 2022

Uh oh!

smanna12 commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Nov 16, 2022

Uh oh!

jchlanda commented Nov 18, 2022

Uh oh!

AaronBallman commented Nov 18, 2022

Uh oh!

jchlanda commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Nov 22, 2022

jchlanda commented Oct 28, 2022 •

edited

Loading

smanna12 commented Nov 16, 2022 •

edited

Loading

jchlanda commented Nov 22, 2022 •

edited

Loading

pvchupin commented Dec 7, 2022 •

edited

Loading