[SYCL] Allow multiple build options for opencl-aot #2828
Conversation
vmaksimo
requested review from
AlexeySachkov,
dm-vodopyanov and
romanovvlad
as code owners
opencl-aot/source/main.cpp
Outdated
| std::string BuildOptions = OptBuildOptions; | ||
| std::string BuildOptsStr; | ||
| if (!OptBuildOptions.empty()) { | ||
| BuildOptsStr = *OptBuildOptions.begin(); |
There was a problem hiding this comment.
Why this can't be handled within a loop below?
There was a problem hiding this comment.
It's done to avoid trailing space delimiter. If we start for_each from the first element, there will be a space at the beginning of the string.
Possibly, it's not a big problem, but I was not sure.
opencl-aot/source/main.cpp
Outdated
| std::string BuildOptions = OptBuildOptions; | ||
| std::string BuildOptsStr; | ||
| if (!OptBuildOptions.empty()) { | ||
| BuildOptsStr = *OptBuildOptions.begin(); |
There was a problem hiding this comment.
Can range-based loop be used here? Smth like that:
if (!OptBuildOptions.empty()) {
for (const auto& BuildOption: OptBuildOptions) {
BuildOptions += BuildOption + ' ';
}
}
There was a problem hiding this comment.
Do we care about the extra space delimiter at the end of the string? If not, then that's an option
There was a problem hiding this comment.
Sorry, missed this thread. Yeah, it doesn't matter if extra space is at the beginning or at the end of the string.
opencl-aot/source/main.cpp
Outdated
| @@ -414,7 +414,14 @@ int main(int Argc, char *Argv[]) { | |||
| CLProgramUPtr ProgramUPtr(std::move(Progs[0])); | |||
|
|
|||
| // step 7: set OpenCL build options | |||
| std::string BuildOptions = OptBuildOptions; | |||
| std::string BuildOptsStr; | |||
There was a problem hiding this comment.
Better to use BuildOptions variable here, the number of variables reduced.
There was a problem hiding this comment.
I thought so too, but on 433 line we use the initial build options string, not the BuildOptions with the parent directory path attached (on line 427). And we, unfortunately, can't reuse OptBuildOptions for this purpose as it was done previously
There was a problem hiding this comment.
Oh, it looks like a bug on my side - BuildOptions should have been passed to clBuildProgram, not OptBuildOptions. That's because on line 427 a mandatory option -I"/path/to/dir" is adding to BuildOptions, and after that it prints to cout, and it doesn't use in any way after that - just another variable - OptBuildOptions - is used in algorithm below. This mandatory option is required for FPGA Emu, and dpcpp don't use opencl-aot for AOT FPGA Emu, but definitely is should be fixed.
Can you please try to use the single variable BuildOptions here?
There was a problem hiding this comment.
Oh, that makes more sense.
Sure, I've uploaded the change you asked for, you're welcome to review it.
opencl-aot/source/main.cpp
Outdated
| std::string BuildOptsStr; | ||
| if (!OptBuildOptions.empty()) { | ||
| BuildOptsStr = *OptBuildOptions.begin(); | ||
| for_each(OptBuildOptions.begin() + 1, OptBuildOptions.end(), |
Exactly, here is the test for this case intel/llvm-test-suite#58 |
This patch adds handling of multiple build options if they were set separately. Signed-off-by: Viktoria Maksimova <[email protected]>
This patch adds handling of multiple build options if they were set separately.
Covered by test https://github.com/intel/llvm-test-suite/pull/58/files
Signed-off-by: Viktoria Maksimova [email protected]