[SYCL] Link device lib via mlink-builtin-bitcode #2833
Conversation
Signed-off-by: gejin <[email protected]>
jinge90
requested review from
AGindinson,
mdtoguchi and
vzakhari
as code owners
Signed-off-by: gejin <[email protected]>
|
@bader |
Signed-off-by: gejin <[email protected]>
|
/summary:run |
@jinge90, could you share a short reproducer for this problem, so I can take a look, please? |
| void SYCLToolChain::addClangTargetOptions( | ||
| const llvm::opt::ArgList &DriverArgs, | ||
| llvm::opt::ArgStringList &CC1Args, | ||
| Action::OffloadKind DeviceOffloadingKind) const { | ||
| if (DeviceOffloadingKind == Action::OFK_SYCL && doLLVMBCEmit(CC1Args)) |
There was a problem hiding this comment.
What is the reason to do doLLVMBCEmit(CC1Args) check?
There was a problem hiding this comment.
Hi, @bader
If we don't do this check, "-mlink-builtin-bitcode ..." will be added when we generate sycl helper header file. We only need to link those device libraries when generating device code.
There was a problem hiding this comment.
We need to link them always. Integration header is generated from Sema, so adding -m* options should have no impact.
There was a problem hiding this comment.
Hi, @bader
Since the "-mlink" options have no impact to integration header generation, why don't remove the unused options to make the commands more clean?
Thanks very much.
There was a problem hiding this comment.
This is not the right place to make this decision. We must always link devicelib unless user explicitly disable the linking. We should have some "optimizing" logic here as it's a error prone. I can image a few cases already where this can lead to bugs.
|
|
||
| bool NoDeviceLibs = false; | ||
| // Currently, libc, libm-fp32 will be linked in by default. In order | ||
| // to use libm-fp64, -fsycl-device-lib=libm-fp64/all should be used. |
There was a problem hiding this comment.
Do you see any problems with linking libm-fp64 by default?
There was a problem hiding this comment.
Hi, @bader
I think it should be OK to link libm-fp64 by default too. I will find a some machine without fp64 extension to see if any problem exposed.
Thanks very much.
There was a problem hiding this comment.
There should be no problems. If math functions with doubles are not used there will be no unsupported functions in the code.
There was a problem hiding this comment.
@bader @mdtoguchi @vzakhari
Have updated the patch to link libm-fp64 by default. But I suggest to re-discuss the behavior of "-fsycl-device-lib=..."
Currently, "-fsycl-device-lib=.." won't impact our "default" link behavior, it seems that current behavior of "-fsycl-device-lib=" is useless after we link all device libraries by default.
Maybe aligning behaviors of "-fsycl-device-lib" and "-fno-sycl-device-lib" is more reasonable, if user addes "-fsycl-device-lib=libm-fp32", we won't do "default" link, instead, we only link libm-fp32 device libraries, the same to libc, libm-fp64...
And I think we should re-consider the "-fsycl-device-lib=..." no matter we choose this PR or #2783 in the end.
Thanks very much.
There was a problem hiding this comment.
I would say we have just one option - -fsycl analog for -nostdlib i.e. -fno-sycl-device-lib.
I don't think we really need a fine grained control over which part of standard library is linked.
There was a problem hiding this comment.
Agree with @bader. We shouldn't modify what we link by default with an option that adjusts the adding of libs. Similar to adding -lgcc on the command line, that doesn't override the default libs already linked in. A single option that disables all is reasonable.
Signed-off-by: gejin <[email protected]>
Signed-off-by: gejin <[email protected]>
Signed-off-by: gejin <[email protected]>
Signed-off-by: gejin <[email protected]>
Signed-off-by: gejin <[email protected]>
Signed-off-by: gejin <[email protected]>
jinge90
changed the title
Hm... this PR was updated more than a year ago and it looks outdated. There are multiple merge conflicts. |
I think we can close it now. |
This is a prototype to implement SYCL device libraries via "-mlink-builtin-bitcode" suggested by Alexey. We have another options: #2783
We need more investigation to decide which way to choose.
Thanks very much.