[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?