[SYCL] Add option to sycl-post-link to embed module properties/symbols as metadata #14197
Conversation
…s as metadata Signed-off-by: Sarnie, Nick <[email protected]>
| bool IsUsingLTO = TC.getDriver().isUsingLTO(/*IsDeviceOffloadAction=*/true); | ||
| auto LTOMode = TC.getDriver().getLTOMode(/*IsDeviceOffloadAction=*/true); | ||
| if (IsUsingLTO && LTOMode == LTOK_Thin) | ||
| addArgs(PostLinkArgs, TCArgs, {"-embed-aux-info-as-metadata"}); |
There was a problem hiding this comment.
This doesn't look like it needs any Triple information to resolve - can you move into getNonTripleBasedSYCLPostLinkOpts?
There was a problem hiding this comment.
oh yeah sorry, fixed in latest commit, thanks.
Signed-off-by: Sarnie, Nick <[email protected]>
There was a problem hiding this comment.
I'm not sure about this approach, so I'd like @AlexeySachkov to review as well.
According to my understanding, sycl-post-link runs llvm pass to obtain SYCL properties and sym-table (i.e. this information is encoded in LLVM IR) and saves it to a separate file, so that SYCL runtime won't need to parse LLVM IR.
Don't clang-linker-wrapper parses LLVM module? If so, it make sense to run the analysis pass in clang-linker-wrapper instead of sycl-post-link tool to avoid an overhead on handling metadata.
| ; In particular, we should see the properties and symbols file are not added to the output table | ||
| ; and are instead embedded in the module as metadata. |
There was a problem hiding this comment.
| ; In particular, we should see the properties and symbols file are not added to the output table | |
| ; and are instead embedded in the module as metadata. | |
| ; In particular, we should see the properties and symbols file are embedded in the module as metadata. |
| if (DoSymGen) { | ||
| // We don't need a seperate file with properties if we saved it as | ||
| // metadata inside the module itself. | ||
| if (!EmbedAuxillaryInfoAsMetadata) |
There was a problem hiding this comment.
Little bit of simplification. We can do,
if (EmbedAuxillaryInfoAsMetadata)
return Res;
followed by original code.
I did have some thoughts in similar lines. I am ok with adding this functionality inside the sycl-post-link. We can try to move it out of the post-link tool and possibly right to the spot where it gets consumed in a future PR. Thanks |
|
Add [thinLTO] to PR topic? |
| @@ -7,3 +7,4 @@ | |||
| // Verify there's no error and we see the expected cc1 flags with the new offload driver. | |||
There was a problem hiding this comment.
| // Verify there's no error and we see the expected cc1 flags with the new offload driver. | |
| // Verify there's no error and we see the expected cc1 and sycl-post-link flags with the new offload driver. |
There was a problem hiding this comment.
Overall LGTM. Few minor comments.
This PR is well-written and is designed such that it is a well-guarded and incremental change. I will wait for consensus on the discussion about where this logic needs to reside to see if anyone identifies a red flag.
From my side, I am fine with the current location and any optimization can be added later.
Thanks @sarnex.
I think that gathering properties later in a pipeline (i.e. within Moreover, thinLTO implies some optimizations, right? It could theoretically invalidate some of those properties due to LLVM IR changes. Moreover, some of the properties (like ones about specialization constants) can only be gathered once we have linked device image - computing them before any kind of linking is not possible. At first glance this PR seems like a workaround for not calling |
|
Thanks for the comments and sorry @AlexeySachkov I must have misunderstood our earlier conversion, I got the impression we decided to do it this way. I'll drop this PR and try to rework how property generation works so we can call it from |
…n into IR-based analysis (#14220) Based on feedback from #14197, I seperated out the code that generates the module properties and symbol table into separate functions that can be called by anyone, and just looks at the IR and entry points. For now, we still call it inside `sycl-post-link` because we still support the old offloading model, but once we drop support for that we can drop this responsibility from sycl-post-link and only compute it inside `clang-linker-wrapper`, both for normal compilation and thinLTO. In a (hopefully soon) future PR I plan to call these functions from `clang-linker-wrapper` when compiling for thinLTO, which we need because we will split early. Most of this change should be NFC(I). The expected changes are: 1) New option to sycl-post-link to generate the properties file 2) Driver change to NOT pass the option from 1) in thinLTO mode 3) Two minor chages in logic from properties generation, I've called these out inline. --------- Signed-off-by: Sarnie, Nick <[email protected]>
…n into IR-based analysis (#14220) Based on feedback from #14197, I seperated out the code that generates the module properties and symbol table into separate functions that can be called by anyone, and just looks at the IR and entry points. For now, we still call it inside `sycl-post-link` because we still support the old offloading model, but once we drop support for that we can drop this responsibility from sycl-post-link and only compute it inside `clang-linker-wrapper`, both for normal compilation and thinLTO. In a (hopefully soon) future PR I plan to call these functions from `clang-linker-wrapper` when compiling for thinLTO, which we need because we will split early. Most of this change should be NFC(I). The expected changes are: 1) New option to sycl-post-link to generate the properties file 2) Driver change to NOT pass the option from 1) in thinLTO mode 3) Two minor chages in logic from properties generation, I've called these out inline. --------- Signed-off-by: Sarnie, Nick <[email protected]>
With thinLTO, we decided to run
sycl-post-linkearly. To do that, we need to be able to figure out the properties and symbol table for each split insideclang-linker-wrapper. Today these are computed insidesycl-post-linkafter splitting, and we need a way to get them insideclang-linker-wrapper. Right nowsycl-post-linkis written such that these are most easily computed on a module that has just been split. It should be possible to change that to only look at a BC file without requiring that it was just split, but it seemed simpler to just keep the current architecture. To do that, we embed the symbol table and module properties into the module when thinLTO is enabled. This information will later be read byclang-linker-wrapper, to be added in a future change.When this option is enabled, we don't create separate files for the properties and symbols and also do not add it to the output table.