[SYCL][Driver] Improve -save-temps for SYCL mode
#4931
Conversation
jchlanda
requested review from
AGindinson,
hchilama and
mdtoguchi
as code owners
jchlanda
force-pushed
the
jakub/save_temps_llvm_foreach
branch
2 times, most recently
from
dc12fa6 to
f1d2f4d
Compare
clang/lib/Driver/Driver.cpp
Outdated
| llvm::sys::path::remove_filename(OutFileDir); | ||
| // `remove_filename` will return an empty string if in CWD, patch it. | ||
| if (OutFileDir.empty()) | ||
| llvm::sys::path::native(OutFileDir = "./"); |
There was a problem hiding this comment.
It is my understanding that we should use the -o information for the saved temporary files when -save-temps=obj is used. Otherwise, the CWD is used.
Also, since OutFileDir is only used with IsSaveTemps, we can move this logic there.
There was a problem hiding this comment.
Agreed.
I kept the declaration of OutFileDir above the loop, so it doesn't need to be re-calculated.
jchlanda
force-pushed
the
jakub/save_temps_llvm_foreach
branch
2 times, most recently
from
bf7a42b to
698272a
Compare
clang/test/Driver/sycl-offload.c
Outdated
| /// ########################################################################### | ||
| /// Verify that -save-temps puts header/footer in a correct place | ||
| // RUN: %clang -fsycl -fno-sycl-device-lib=all -fsycl-targets=spir64-unknown-unknown -target x86_64-unknown-linux-gnu -save-temps %s -### 2>&1 | FileCheck %s -check-prefixes=CHECK-SAVE-TEMPS-DIR | ||
| // CHECK-SAVE-TEMPS-DIR: clang{{.*}} "-fsycl-int-header=./sycl-offload-header-{{[a-z0-9]*}}.h"{{.*}}"-fsycl-int-footer=./sycl-offload-footer-{{[a-z0-9]*}}.h" |
There was a problem hiding this comment.
Can you also check behaviors with -save-temps=obj and -o?
There was a problem hiding this comment.
Interestingly enough, -save-temps=obj with -o containing a relative path breaks the compilation. It fails to correctly construct the output file names for sycl-post-link job. As that is also present in main branch, I'd like to address it in a separate PR.
jchlanda
force-pushed
the
jakub/save_temps_llvm_foreach
branch
from
ca2f3ca to
9a19200
Compare
clang/lib/Driver/Driver.cpp
Outdated
| // If in `SaveTempsCwd` mode, or `remove_filename` returned an empty | ||
| // string use CWD, otherwise append separator. | ||
| if (OutFileDir.empty()) | ||
| llvm::sys::path::native(OutFileDir = "./"); |
There was a problem hiding this comment.
I looked through how other temporaries for save-temps are formulated in regards to $CWD, and there is no qualifier like ./. The output is just the filename. Perhaps we should do the same here.
There was a problem hiding this comment.
I think you are right, removed. Looks like the other use of ./ https://github.com/jchlanda/llvm/blob/jakub/save_temps_llvm_foreach/clang/lib/Driver/ToolChains/SYCL.cpp#L138 will have to stay, because of the way llvm-foreach handles the OutDirectory option.
jchlanda
force-pushed
the
jakub/save_temps_llvm_foreach
branch
from
7734610 to
e91f9d4
Compare
|
Is there anything else that should be done for this PR? |
bader
changed the title
save-temps improvements-save-temps for SYCL mode
jchlanda
force-pushed
the
jakub/save_temps_llvm_foreach
branch
from
e91f9d4 to
a9d40c8
Compare
|
@jchlanda, please, take a look at test failures on Windows platform. |
Co-authored-by: mdtoguchi <[email protected]>
jchlanda
force-pushed
the
jakub/save_temps_llvm_foreach
branch
from
41ae583 to
1f3e9da
Compare
Make sure that
headerandfooterfiles as well as the output ofllvm-foreach(such asasmandfatbinfor CUDA) stay in the output directory.As discussed here: #4752
I'm unsure if that is a canonical way of getting the directory, as it relies on the output file's directory (not absolute) or the current dir.