[SYCLPostLink] Accept 2-column split table when -symbols is omitted

[wenju-he]

parseSplitModulesFromFile required the 3-column
"[Code|Properties|Symbols]" table unconditionally, but clang-linker-wrapper omits -symbols when -sycl-thin-lto is set, so sycl-post-link emits a 2-column "[Code|Properties]" table instead. This made the parser reject the table with "invalid SYCL Table file.", aborting the link for -foffload-lto=thin. Accept both header/row shapes, leaving Symbols empty when the column is absent.

[bader]

parseSplitModulesFromFile required the 3-column "[Code|Properties|Symbols]" table unconditionally, but clang-linker-wrapper omits -symbols when -sycl-thin-lto is set, so sycl-post-link emits a 2-column "[Code|Properties]" table instead. This made the parser reject the table with "invalid SYCL Table file.", aborting the link for -foffload-lto=thin. Accept both header/row shapes, leaving Symbols empty when the column is absent.

@wenju-he, do you have a design doc for enabling ThinLTO in SYCL mode?
From my POV, ThinLTO should not need sycl-post-link functionality. Is this a temporal change to serve as a POC?

[wenju-he]

parseSplitModulesFromFile required the 3-column "[Code|Properties|Symbols]" table unconditionally, but clang-linker-wrapper omits -symbols when -sycl-thin-lto is set, so sycl-post-link emits a 2-column "[Code|Properties]" table instead. This made the parser reject the table with "invalid SYCL Table file.", aborting the link for -foffload-lto=thin. Accept both header/row shapes, leaving Symbols empty when the column is absent.

@wenju-he, do you have a design doc for enabling ThinLTO in SYCL mode?

I have attached a diagram of current implementation (almost finished and I'm splitting it into smaller patches for review) in jira CMPLRLLVM-78079.

From my POV, ThinLTO should not need sycl-post-link functionality. Is this a temporal change to serve as a POC?

I agree with you. See #14259 for background that adds per-TU sycl-post-link for ThinLTO.
This PR and other sycl-post-link related changes in my implementation is just to not break existing tests.
I can see some logic in sycl-post-link that probably doesn't make much sense now, e.g. requiring uniform required_subgroup_size in SPLIT_PER_TU.
In ThinLTO, there isn't really a full post-link step on LLVM IR. Maybe we should deprecate sycl-post-link functionality if possible. There is probably no equivalent functionality in AMDGPU pipeline.

[wenju-he]

Is this a temporal change to serve as a POC?

Not temporary, but can be reverted when sycl-post-link is deprecated, which is out of scope of current ThinLTO implementation.

[mdtoguchi]

Looks OK to me. As an aside, looked through the existing LIT tests and could not find anything that is verifying the -sycl-thin-lto behavior for clang-linker-wrapper in that it restricts the Symbols column for the sycl-post-link call.

[sys-ce-bb]

@intel/llvm-gatekeepers please consider merging

[bader]

In ThinLTO, there isn't really a full post-link step on LLVM IR. Maybe we should deprecate sycl-post-link functionality if possible. There is probably no equivalent functionality in AMDGPU pipeline.

I don't think we can deprecate sycl-post-link functionality required for implementing SYCL. Instead, we should move this functionality to ThinLTO pipeline. My understanding all sycl-post-link functionality is implemented in LLVM IR passes, which require "linked" LLVM module. It should be possible to run these passes at post-link phase of LTO pipeline.

The trickiest part to integrate into LTO pipeline is module splitting functionality. AFAIK, AMDGPU pipeline has implemented similar splitting in the CodeGen, but heuristic they use is different. We can probably integrate split functionality into SPIR-V backend to get per-kernel and per-aspect. ThinLTO framework naturally does per-source split already.

Deprecating sycl-post-link should not block enabling ThinLTO support. ThinLTO can implement the compilation pipeline parallel to the existing one independently.

[wenju-he]

In ThinLTO, there isn't really a full post-link step on LLVM IR. Maybe we should deprecate sycl-post-link functionality if possible. There is probably no equivalent functionality in AMDGPU pipeline.

I don't think we can deprecate sycl-post-link functionality required for implementing SYCL. Instead, we should move this functionality to ThinLTO pipeline. My understanding all sycl-post-link functionality is implemented in LLVM IR passes, which require "linked" LLVM module. It should be possible to run these passes at post-link phase of LTO pipeline.

The trickiest part to integrate into LTO pipeline is module splitting functionality. AFAIK, AMDGPU pipeline has implemented similar splitting in the CodeGen, but heuristic they use is different. We can probably integrate split functionality into SPIR-V backend to get per-kernel and per-aspect. ThinLTO framework naturally does per-source split already.

Deprecating sycl-post-link should not block enabling ThinLTO support. ThinLTO can implement the compilation pipeline parallel to the existing one independently.

I agree, But I think we can implement this incrementally.
At least, #14259 would need to reverted, right? But I think that is out of scope for the initial ThinLTO implementation. We don't need to be perfect for the first implementation. The core part of ThinLTO is cross-TU summary-based linking and parallel codegen, and none of sycl-post-link issue is related to this core part. We can refine the sycl-post-link functionality in ThinLTO later.

[bader]

At least, #14259 would need to reverted, right?

Probably. I think @sarnex landed this patch before AMD folks pushed splitting capabilities the upstream branch.
According to my understanding, #14259 implements only small part of the thinLTO support for SYCL. @sarnex, did you have fully working SYCL compiler with thinLTO support at the time of merging #14259?

[sarnex]

It looks like the final AMDGPU splitting support was merged in May 2024 where as my change was merged in June 2024, although some infra work for the AMDGPU splitting support was merged earlier in April 2024.

I don't think I knew about the AMDGPU splitting support at the time, at the time we had a looser relationship with upstream than we do today.

I never merged full working SYCL thinLTO support, I had a prototype under-review here, but eventually I decided to delay working on it because the new offload model device linking flow for SYCL support in intel/llvm was so different from upstream at the time and a lot of changes were required, I didn't see it as a good use of resources to get it working when it would all have to be all redone when we unify the device linking flow with upstream. I think we are much closer now.

I don't think we can deprecate sycl-post-link functionality required for implementing SYCL. Instead, we should move this functionality to ThinLTO pipeline. My understanding all sycl-post-link functionality is implemented in LLVM IR passes, which require "linked" LLVM module. It should be possible to run these passes at post-link phase of LTO pipeline.

I agree with this, we shouldn't need to call the sycl-post-link tool in the thinLTO flow, but we will definitely need its logic, ideally these are just passes we can run.

Note that at least when I was working on it, the specialization constants feature required running a special pass on fully linked module, if the module was not fully linked, the feature would not work correctly. I'm not sure if this is still true, maybe the feature changed. But this is an important case to consider, some language features may require processing a fully linked module.

At least, #14259 would need to reverted, right?

Right, we should use the new community mechanism for splitting, we should not rely on sycl-post-link's existing logic to do it (IMO this is true even for non-thinLTO 😜). Doing this incrementally is fine with me, where we use the logic in #14259 today and later move to using the community mechanism.

[wenju-he]

Thanks @sarnex for the suggestions. We can do migration from the current per-TU sycl-post-link split (used to produce ThinLTO's per-Task inputs) to upstream splitModuleTransitiveFromEntryPoints as a follow-up step for ThinLTO. It might not be trivial work since sycl-post-link does more than splitModuleTransitiveFromEntryPoints, doing it separately also allow us to monitor regressions.

[wenju-he]

@intel/llvm-gatekeepers please merge, thanks