New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement llvm-foreach tool #793

Merged

romanovvlad merged 7 commits into intel:sycl from Fznamznon:private/mpodchis/xargs

Nov 21, 2019

Contributor

Fznamznon commented Nov 6, 2019 •

edited

Loading

The tool has simple functionality: execute specified command for each
file in input file list.
The tool will be used in the clang driver for cases when number of
action's outputs is unknown and used tools don't support file lists.

Signed-off-by: Mariya Podchishchaeva mariya.podchishchaeva@intel.com

Fznamznon requested review from kbobrovs and asavonic

November 6, 2019 13:33

asavonic reviewed

View reviewed changes

Contributor

asavonic left a comment

Please state the motivation for this patch in the commit message (and in the PR description).

The tool looks good overall, and seems to do its job. However, the its command line interface is totally different from *nix xargs. I think that the tool should either be renamed (llvm-foreach?), or (probably better) its interface should be aligned with xargs.

llvm/tools/llvm-xargs/llvm-xargs.cpp Outdated

    
              static cl::opt<std::string> OutReplace{

                  "out-replace",

                  cl::desc("Replace R in command with temporary filename for writing outputs "

Contributor

asavonic Nov 6, 2019

Just by looking at the help message, I cannot figure out what this option does.

Contributor Author

Fznamznon Nov 14, 2019

Tried to improve.

llvm/tools/llvm-xargs/llvm-xargs.cpp Outdated Show resolved Hide resolved

llvm/tools/llvm-xargs/llvm-xargs.cpp Outdated Show resolved Hide resolved

Contributor

keryell commented Nov 7, 2019

It is not clear why you need to reimplement xargs if it is just like xargs...
Just install xargs on the system? Recompile it on Windows? Use WSL? Use a .bat script doing the same?

Contributor Author

Fznamznon commented Nov 8, 2019 •

edited

Loading

It is not clear why you need to reimplement xargs if it is just like xargs...
Just install xargs on the system? Recompile it on Windows? Use WSL? Use a .bat script doing the same?

The idea was taken from unix xargs but I'm not sure it's exactly xargs. It's xargs with additional functionality - generation of output file list. I think, I'm going to rename it to something like "llvm-foreach" as @asavonic suggests...

Actually it's needed for SYCL device code split feature #631 .
Basically split action will have multiple outputs and number of these outputs is unknown. All these outputs should be passed through some part of SYCL toolchain by the clang driver. The problem is in clang driver. When number of outputs is unknown before compilation invocation, the clang driver can operate only with file lists. But a lot of compilation tools don't have support for file lists, so we are implementing a new tool which will execute other tools for each file from specified list.
So, this tool (llvm-foreach?) will be used in the clang driver to execute tools which don't have support for multiple inputs/filelists. So, to make this tool working with the clang driver on any system, we just implement it.
I'll add this information to commit message soon.

agozillon mentioned this pull request

[SYCL][Driver] Refactor xocc driver script back into the LLVM Driver/ToolChain triSYCL/sycl#73

Open

Fznamznon force-pushed the private/mpodchis/xargs branch from 2207d3a to ad83423 Compare

November 14, 2019 16:40

Fznamznon changed the title ~~Implement llvm-xargs tool~~ Implement llvm-foreach tool

Fznamznon assigned asavonic

Contributor Author

Fznamznon commented Nov 14, 2019

Applied comments.
Also I added support of multiple file lists and arguments containing "=".

asavonic reviewed

View reviewed changes

llvm/tools/llvm-foreach/llvm-foreach.cpp Show resolved Hide resolved

asavonic reviewed

View reviewed changes

llvm/tools/llvm-foreach/CMakeLists.txt Outdated Show resolved Hide resolved

asavonic reviewed

View reviewed changes

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated Show resolved Hide resolved

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated Show resolved Hide resolved

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated Show resolved Hide resolved

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated Show resolved Hide resolved

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated Show resolved Hide resolved

Fznamznon force-pushed the private/mpodchis/xargs branch from 54ad0e5 to 95e90fe Compare

November 18, 2019 08:04

asavonic suggested changes

View reviewed changes

llvm/tools/llvm-foreach/llvm-foreach.cpp Show resolved Hide resolved

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated

    
                ArgumentReplace OutReplaceArg;

                for (size_t i = 0; i < Args.size(); ++i) {

                  for (auto &Replace : Replaces)

                    if (Args[i].contains(Replace)) {

Contributor

asavonic Nov 19, 2019

Search for the same substring in the same string happens 3 times: here in contains, in Arg.find and then in Arg.find_last_of.

Contributor Author

Fznamznon Nov 19, 2019

Now it's 1 time.

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated

    
                struct ArgumentReplace {

                  int ArgNum = -1;

                  std::string Prefix;

Contributor

asavonic Nov 19, 2019

It is enough to just store an index where Replace string starts, and it's length.
No need to split a string into two allocations.

Contributor Author

Fznamznon Nov 19, 2019

Okay, done.

llvm/tools/llvm-foreach/llvm-foreach.cpp Show resolved Hide resolved

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated

    
                  int NumOfLines = 0;

                  for (; !LineIterators[i].is_at_eof(); ++LineIterators[i]) {

                    FileLists[i].push_back(LineIterators[i]->str());

                    NumOfLines++;

Contributor

asavonic Nov 19, 2019

NumOfLines seems to be the same as FileLists[i].size().

Contributor Author

Fznamznon Nov 19, 2019

Yep. Removed NumOfLines.

llvm/tools/llvm-foreach/llvm-foreach.cpp Outdated

    
                    Args[OutReplaceArg.ArgNum] = ResOutArg;

                    if (!OutputFileList.empty())

                      ResFileList = (Twine(ResFileList) + Twine(Path) + Twine("\n")).str();

Contributor

asavonic Nov 19, 2019

There is no need to keep concatenating file names into a string, you can just write them to OutputFileList file.

Contributor Author

Fznamznon Nov 19, 2019

Okay, done.

Fznamznon added 7 commits

November 19, 2019 15:16


          Implement llvm-foreach tool

fe64e41

The tool has simple functionality: execute specified command for each
file in input file list.
The tool will be used in the clang driver for cases when number of
action's outputs is unknown and used tools don't support file lists.

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>


          Apply CR comments

643d68c

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>


          Fix CMakeLists

d901cf2

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>


          Apply CR suggestion

ebca2db

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>


          Update tests

3c28262

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>


          Return back "--out-ext" option

6e6abe9

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>


          Apply CR comments

624944e

Signed-off-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fznamznon force-pushed the private/mpodchis/xargs branch from 8767104 to 624944e Compare

November 19, 2019 14:46

asavonic approved these changes

View reviewed changes

Contributor

asavonic left a comment

Just to be clear: I strongly believe that the clang driver is supposed to handle this use case, and making a tool to cover this particular deficiency is a workaround.
Having said that, I've no major objections to the code itself.

Contributor Author

Fznamznon commented Nov 20, 2019

Just to be clear: I strongly believe that the clang driver is supposed to handle this use case, and making a tool to cover this particular deficiency is a workaround.
Having said that, I've no major objections to the code itself.

Yes, It's some kind of workaround now. But I'm afraid that to support such use cases clang driver needs some significant changes, probably redesign. As you can see, not only we faced such problems with the driver - triSYCL/sycl#73 .
So, I think we can maybe try to start a talk with llvm community about it.

Fznamznon assigned romanovvlad

Fznamznon mentioned this pull request

[SYCL][Driver] Add device code split job #853

Merged

Contributor Author

Fznamznon commented Nov 21, 2019

Ping.

romanovvlad merged commit 0543397 into intel:sycl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet