[SYCL] Add splitting module capabilities when compiling for NVPTX and AMDGCN #4107
Conversation
|
Am I right that this will help to resolve #3833, because |
Not tested or looked into it, but should as it makes use of the tools and frmat brought for SYCL. |
…ing binding. The patch adds ForEachWrappingClass which allows to encapsulate a set of actions whose underlying commands will be wrapped under a llvm-foreach commands. During the binding phases, the action will trigger the emission of jobs outside the set, record the Jobs state then triggers the emission of the jobs inside the set. The newly added commands are stolen from the Jobs list and the commands are re-emitted with the llvm-foreach wrapping. Signed-off-by: Victor Lomuller <victor@codeplay.com>
…PTX or AMDGCN. The patch allows modules splitting for NVPTX and AMDGCN targets. The driver works by wrapping the actions with a ForEachWrappingAction. To allow standard clang tools to work properly, SYCLPostLinkJobAction and FileTableTformJobAction reports the underlying file type rather than TY_Tempfilelist or TY_tempfiletable for those targets. Signed-off-by: Victor Lomuller <victor@codeplay.com>
Naghasan
force-pushed
the
victor/nvptx-amdgcn-splitmodule
branch
from
d792d55 to
84966b2
Compare
There was a problem hiding this comment.
During the binding phases, the action will trigger the emission of jobs outside the set,
record the Jobs state then triggers the emission of the jobs inside the set.
The newly added commands are stolen from the Jobs list and
the commands are re-emitted with the llvm-foreach wrapping.
Overall, this approach LGTM personally - rather slick for the existing infrastructure! I have yet to review the "main" working code inside Driver.cpp, meanwhile, welcoming others to chime in with their review of the design (@mdtoguchi, @kbobrovs?)
To not make this patch too large, I maintained the SPIR-V path behaviour as it is, meaning the list of actions are not set up in the same way nor does it uses the ForEachWrappingAction. Otherwise there is a long list of test to update.
Fair point; that deserves a separate PR for sure (a TODO or a new GH issue could seal this part for the current PR). To my mind, having SPIR target printouts show the actual file extensions is worth the change, yet again, opinions from the architects would be definitive.
Signed-off-by: Victor Lomuller <victor@codeplay.com>
Signed-off-by: Victor Lomuller <victor@codeplay.com>
|
@AGindinson, @mdtoguchi, could you take a look at this PR, please? |
There was a problem hiding this comment.
LGTM besides the typos; I'd appreciate an approval from @mdtoguchi
Co-authored-by: Artem Gindinson <artem.gindinson@intel.com>
There was a problem hiding this comment.
Sorry, stumbled upon another typo. Also, is it possible to enable CUDA tests in https://github.com/intel/llvm-test-suite/blob/intel/SYCL/DeviceCodeSplit in a follow-up PR so that we have an additional E2E test happening?
| @@ -737,7 +738,15 @@ class SYCLPostLinkJobAction : public JobAction { | |||
| void anchor() override; | |||
|
|
|||
| public: | |||
| SYCLPostLinkJobAction(Action *Input, types::ID OutputType); | |||
| // The tempfiletable management relies on a shadowing the main file type by | |||
There was a problem hiding this comment.
| // The tempfiletable management relies on a shadowing the main file type by | |
| // The tempfiletable management relies on shadowing the main file type by |
There was a problem hiding this comment.
I'm not 100% sure that using article in this case is incorrect.
There was a problem hiding this comment.
Overall - LGTM. Can you also update the device flow here: https://github.com/intel/llvm/blob/sycl/sycl/doc/images/DeviceLinkAndWrap.svg
|
I suggest we commit this fix and address documentation and additional testing comments in follow-up commit. |
… AMDGCN (intel#4107) The patch allows modules splitting for NVPTX and AMDGCN targets. As those path needs to rely on existing tool, the approach taken here is to generate and intercept commands so they can be wrapped by a `llvm-foreach` tool call. To do that, the patch adds `ForEachWrappingAction` which allows to encapsulate a set of actions whose underlying commands will be wrapped under a llvm-foreach commands. During the binding phases, the action will trigger the emission of jobs outside the set, record the Jobs state then triggers the emission of the jobs inside the set. The newly added commands are stolen from the Jobs list and the commands are re-emitted with the llvm-foreach wrapping. To allow standard clang tools to work properly, SYCLPostLinkJobAction and FileTableTformJobAction reports the underlying file type rather than TY_Tempfilelist or TY_tempfiletable for those targets. Otherwise tools get confused as they don't know how to process the input. To not make this patch too large, I maintained the SPIR-V path behaviour as it is, meaning the list of actions are not set up in the same way nor does it uses the `ForEachWrappingAction`. Otherwise there is a long list of test to update. Signed-off-by: Victor Lomuller <victor@codeplay.com>
The patch allows modules splitting for NVPTX and AMDGCN targets.
As those path needs to rely on existing tool, the approach taken here is
to generate and intercept commands so they can be wrapped by a
llvm-foreachtool call.To do that, the patch adds
ForEachWrappingActionwhich allows to encapsulatea set of actions whose underlying commands will be wrapped under a llvm-foreach commands.
During the binding phases, the action will trigger the emission of jobs outside the set,
record the Jobs state then triggers the emission of the jobs inside the set.
The newly added commands are stolen from the Jobs list and
the commands are re-emitted with the llvm-foreach wrapping.
To allow standard clang tools to work properly,
SYCLPostLinkJobAction and FileTableTformJobAction reports
the underlying file type rather than TY_Tempfilelist or TY_tempfiletable
for those targets. Otherwise tools get confused as they don't know how to process the input.
To not make this patch too large, I maintained the SPIR-V path behaviour as it is, meaning the list of actions are not set up in the same way nor does it uses the
ForEachWrappingAction. Otherwise there is a long list of test to update.I'm open to do it (as part of this patch or a follow up), but I wanted to get feedback on this first.