-
Notifications
You must be signed in to change notification settings - Fork 791
Implement llvm-foreach tool #793
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just by looking at the help message, I cannot figure out what this option does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to improve.
|
It is not clear why you need to reimplement |
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 . |
2207d3a to
ad83423
Compare
|
Applied comments. |
54ad0e5 to
95e90fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's 1 time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumOfLines seems to be the same as FileLists[i].size().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Removed NumOfLines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to keep concatenating file names into a string, you can just write them to OutputFileList file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, done.
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 <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
8767104 to
624944e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 . |
|
Ping. |
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 [email protected]