Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL] Link sycl-devicelib-host lib in clang-cl + fscyl by default. #6503

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Aug 1, 2022

Signed-off-by: jinge90 ge.jin@intel.com

@jinge90 jinge90 requested a review from a team as a code owner August 1, 2022 05:26
@jinge90
Copy link
Contributor Author

jinge90 commented Aug 1, 2022

Hi, @mdtoguchi
Previously, we add sycl-devicelib-host.a in link list by default in if users are using "clang++ -fsycl". This default linked static library consists of implementation of imf libdevice functions for host device. However, we missed adding it in link list in clang-cl mode, this PR adds this static library if users are using "clang-cl -fsycl".

Thanks very much.

@@ -142,6 +142,9 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
CmdArgs.push_back("-defaultlib:sycl-devicelib-host.lib");
}

if (C.getDriver().IsCLMode() && Args.hasArg(options::OPT_fsycl))
CmdArgs.push_back("-defaultlib::sycl-devicelib-host.lib");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are good, can we add a test to check as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @mdtoguchi
Test is added, could you review again?
Thanks very much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason why this lib is being added to the link command line and not being added to the default directives of the generated host object via --dependent-lib=sycl-device-lib-host?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @mdtoguchi
Just checked the code, “dependent-lib=sycl-devicelib-host” is the right approach, I will update the patch.

Thanks very much.

Signed-off-by: jinge90 <ge.jin@intel.com>
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@jinge90 jinge90 requested a review from mdtoguchi August 3, 2022 01:19
@jinge90
Copy link
Contributor Author

jinge90 commented Aug 3, 2022

Thanks! LGTM

Hi, @mdtoguchi
Sorry for bothering you again, I carelessly clicked the button and missed your approval.

Signed-off-by: jinge90 <ge.jin@intel.com>
@jinge90
Copy link
Contributor Author

jinge90 commented Aug 3, 2022

Hi, @tfzhu
It seems that the failure from "SYCL / Linux / CUDA LLVM Test Suite (pull_request)" is not related to my patch, it this a known issue?
Thanks very much.

@tfzhu
Copy link
Contributor

tfzhu commented Aug 3, 2022

Hi, @tfzhu It seems that the failure from "SYCL / Linux / CUDA LLVM Test Suite (pull_request)" is not related to my patch, it this a known issue? Thanks very much.

I have no idea. Maybe @bader or @pvchupin can give some comments.

@bader
Copy link
Contributor

bader commented Aug 3, 2022

Hi, @tfzhu It seems that the failure from "SYCL / Linux / CUDA LLVM Test Suite (pull_request)" is not related to my patch, it this a known issue? Thanks very much.

@jinge90, if you can't find it in https://github.com/intel/llvm/issues, please, open a new issue.

@jinge90
Copy link
Contributor Author

jinge90 commented Aug 4, 2022

Hi, @pvchupin
All tests pass, could you help merge?

Thanks very much.

@pvchupin pvchupin merged commit 221f9c8 into intel:sycl Aug 4, 2022
iclsrc pushed a commit that referenced this pull request Sep 19, 2024
Under some conditions, a trivial `mov xN xN` instruction was emitted on
tail calls. Consider the following code:

```
class Test {
public:
  virtual void f() {}
};

void call_f(Test *t) {
  t->f();
}
```

Correponding assembly:

```
_Z6call_fP4Test:
        ldr     x16, [x0]
        mov     x17, x0
        movk    x17, #6503, lsl #48
        autda   x16, x17
        ldr     x1, [x16]
 =====> mov     x16, x16
        movk    x16, #54167, lsl #48
        braa    x1, x16
```

This patch makes such movs being omitted.

Co-authored-by: Anatoly Trosinenko <atrosinenko@accesssoftek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants